feat(web): version history #1409
Conversation
WalkthroughThe pull request introduces version-management enhancements throughout the codebase. A new Changes
Sequence Diagram(s)sequenceDiagram
participant CD as ContentDetails
participant CH as Versions Hook
participant GQL as GraphQL API
participant CF as ContentForm
participant UI as User
CD->>CH: Request version data (via itemId)
CH->>GQL: Execute VersionsByItemQuery
GQL-->>CH: Return version data
CH->>CH: Process data using fromGraphQLversionsByItem
CH-->>CD: Provide versions
CD->>CF: Pass versions prop to ContentForm
UI->>CF: Select "History Version" Tab
CF-->>UI: Render version history view
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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
CodeRabbit Configuration File (
|
✅ Deploy Preview for reearth-cms ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
web/src/components/molecules/Content/types.ts (1)
97-105: Consider using a more specific type for therequestsfield.The
requestsfield type could be more specific to version-related request properties. Consider whether all request properties are needed in the version history context.- requests: Pick<Request, "id" | "title">[]; + requests: Array<{ + id: string; + title: string; + }>;This approach:
- Makes the type more explicit about what data is expected
- Reduces coupling with the
Requesttype- Improves readability for developers
web/src/components/molecules/Content/Form/SidebarWrapper.tsx (1)
19-32: Consider simplifying thegetStatusBadgefunction.The function could be more concise while maintaining readability.
- const getStatusBadge = (status: ItemStatus) => { - const itemStatus = status.split("_") as StateType[]; - return ( - <> - {itemStatus.map((state, index) => ( - <StyledBadge - key={index} - color={stateColors[state]} - text={index === itemStatus.length - 1 ? t(state) : undefined} - /> - ))} - </> - ); - }; + const getStatusBadge = (status: ItemStatus) => ( + <> + {status.split("_").map((state, index, array) => ( + <StyledBadge + key={state} + color={stateColors[state as StateType]} + text={index === array.length - 1 ? t(state) : undefined} + /> + ))} + </> + );web/src/components/organisms/Project/Content/hooks.ts (1)
61-63: LGTM! Consistent refetch behavior across mutations.All mutations now properly refresh the necessary queries, ensuring data consistency.
Consider extracting the refetch queries array.
The same array of queries is repeated three times. Consider extracting it to a constant.
+const VERSION_RELATED_QUERIES = ["SearchItem", "GetItem", "VersionsByItem"] as const; + const [updateRequest] = useUpdateRequestMutation({ - refetchQueries: ["SearchItem", "GetItem", "VersionsByItem"], + refetchQueries: VERSION_RELATED_QUERIES, }); const [publishItem, { loading: publishLoading }] = usePublishItemMutation({ - refetchQueries: ["SearchItem", "GetItem", "VersionsByItem"], + refetchQueries: VERSION_RELATED_QUERIES, }); const [unpublishItem, { loading: unpublishLoading }] = useUnpublishItemMutation({ - refetchQueries: ["SearchItem", "GetItem", "VersionsByItem"], + refetchQueries: VERSION_RELATED_QUERIES, });Also applies to: 97-99, 118-120
web/src/gql/queries/item.ts (1)
153-196: Consider optimizing the request items query.The query fetches all request items and then filters them in the client-side converter function. Consider optimizing by:
- Adding arguments to filter requests at the GraphQL level.
- Using GraphQL fragments to reduce duplication of Integration/User type fields.
Example optimization:
export const VERSIONS_BY_ITEM = gql` + fragment ActorFields on Actor { + ... on Integration { + name + } + ... on User { + name + } + } + query VersionsByItem($itemId: ID!) { versionsByItem(itemId: $itemId) { version refs value { id modelId status createdAt updatedAt - createdBy { - ... on Integration { - name - } - ... on User { - name - } - } - updatedBy { - ... on Integration { - name - } - ... on User { - name - } - } + createdBy { + ...ActorFields + } + updatedBy { + ...ActorFields + } requests { id title items { itemId version item { value { modelId } } } } } } } `;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
web/src/components/molecules/Content/Details/index.tsx(4 hunks)web/src/components/molecules/Content/Form/SidebarWrapper.tsx(1 hunks)web/src/components/molecules/Content/Form/index.tsx(9 hunks)web/src/components/molecules/Content/types.ts(2 hunks)web/src/components/molecules/Content/utils.ts(1 hunks)web/src/components/organisms/DataConverters/content.ts(3 hunks)web/src/components/organisms/Project/Content/ContentDetails/hooks.ts(6 hunks)web/src/components/organisms/Project/Content/ContentDetails/index.tsx(2 hunks)web/src/components/organisms/Project/Content/ContentList/hooks.ts(1 hunks)web/src/components/organisms/Project/Content/hooks.ts(3 hunks)web/src/gql/graphql-client-api.tsx(2 hunks)web/src/gql/queries/item.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: ci-web / ci
- GitHub Check: Redirect rules - reearth-cms
- GitHub Check: Header rules - reearth-cms
- GitHub Check: Pages changed - reearth-cms
🔇 Additional comments (13)
web/src/components/molecules/Content/utils.ts (1)
4-4: LGTM! Good use of TypeScript's built-in utility type.The change to use
Record<StateType, ColorType>improves code readability while maintaining the same functionality.web/src/components/molecules/Content/Form/SidebarWrapper.tsx (1)
12-14: LGTM! Clean removal of linked requests functionality.The simplified Props type makes the component more focused and maintainable.
web/src/components/organisms/Project/Content/ContentDetails/index.tsx (1)
24-25: LGTM!Clean implementation of prop drilling for the
versionsdata. The changes follow React best practices.Also applies to: 145-145
web/src/components/molecules/Content/Details/index.tsx (1)
9-14: LGTM!Clean implementation of TypeScript types and prop definitions for the version history feature.
Also applies to: 39-39
web/src/components/organisms/Project/Content/ContentDetails/hooks.ts (3)
589-593: LGTM! Query implementation follows best practices.The
useVersionsByItemQueryimplementation:
- Uses appropriate fetch policy for real-time data.
- Properly skips the query when
itemIdis not available.- Follows React Query patterns.
595-601: LGTM! Version data transformation is optimized.The version data transformation:
- Uses
useMemofor performance optimization.- Reverses the array to show latest versions first.
- Safely handles undefined data.
299-299: LGTM! Proper data consistency handling.Adding "VersionsByItem" to
refetchQueriesensures version data is refreshed after item updates.web/src/components/organisms/Project/Content/ContentList/hooks.ts (1)
607-613: Verify if removingrefetchfrom dependency array is intentional.The
refetchfunction has been removed from the dependency array ofhandleBulkAddItemToRequest. While this might prevent unnecessary re-renders, please ensure that:
- The function doesn't need to react to changes in the
refetchfunction.- The data is being refreshed through other means (e.g., through the mutation's
refetchQueries).web/src/components/molecules/Content/Form/index.tsx (2)
748-802: LGTM! Clean and organized tab implementation.The tab implementation:
- Clearly separates metadata and version history.
- Uses conditional rendering for version history tab.
- Follows consistent design patterns.
894-950: LGTM! Well-structured styled components.The styled components:
- Follow a clean and focused approach.
- Use semantic naming.
- Implement proper styling for version history UI.
web/src/gql/graphql-client-api.tsx (3)
2173-2178: LGTM! Well-structured type definition for versioned items.The
VersionedItemtype is well-designed with appropriate fields for version tracking, including version string, references array, optional parents array, and the actual item value.
2573-2575: LGTM! Clean and focused query variables type.The
VersionsByItemQueryVariablestype is appropriately defined with just the requireditemIdparameter.
2827-2903: LGTM! Comprehensive query implementation for version history.The
VersionsByItemQueryis well-implemented with:
- Complete version metadata (version, refs, parents)
- Detailed item information including status, timestamps, and user data
- Proper nesting of related request information
| createdAt: version.value.createdAt, | ||
| updatedAt: version.value.updatedAt, | ||
| createdBy: { name: version.value.createdBy?.name ?? "" }, | ||
| updatedBy: { name: version.value.createdBy?.name ?? "" }, |
There was a problem hiding this comment.
Fix incorrect property access for updatedBy name.
The updatedBy.name is incorrectly using createdBy instead of updatedBy.
Apply this diff to fix the property access:
- updatedBy: { name: version.value.createdBy?.name ?? "" },
+ updatedBy: { name: version.value.updatedBy?.name ?? "" },📝 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.
| updatedBy: { name: version.value.createdBy?.name ?? "" }, | |
| updatedBy: { name: version.value.updatedBy?.name ?? "" }, |
| export const fromGraphQLversionsByItem = ( | ||
| GQLVersionsByItem: GQLVersionedItem[], | ||
| ): VersionedItem[] => { | ||
| let publicIndex = -1; | ||
| const result = GQLVersionsByItem.map<VersionedItem>((version, index) => { | ||
| let status: StateType = "DRAFT"; | ||
| if (version.refs.includes("public")) { | ||
| status = "PUBLIC"; | ||
| publicIndex = index; | ||
| } | ||
| return { | ||
| version: version.version, | ||
| status, | ||
| createdAt: version.value.createdAt, | ||
| updatedAt: version.value.updatedAt, | ||
| createdBy: { name: version.value.createdBy?.name ?? "" }, | ||
| updatedBy: { name: version.value.createdBy?.name ?? "" }, | ||
| requests: | ||
| version.value.requests | ||
| ?.filter(request => | ||
| request.items.some( | ||
| item => | ||
| item.item?.value.modelId === version.value.modelId && | ||
| item.itemId === version.value.id && | ||
| item.version === version.version, | ||
| ), | ||
| ) | ||
| .map(request => ({ | ||
| id: request.id, | ||
| title: request.title, | ||
| })) ?? [], | ||
| }; | ||
| }).map((version, index) => { | ||
| if (index > publicIndex && version.requests.length) { | ||
| version.status = "REVIEW"; | ||
| } | ||
| return version; | ||
| }); | ||
|
|
||
| return result; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for undefined or null values.
The function should handle potential undefined or null values from the GraphQL response to prevent runtime errors.
Apply this diff to add error handling:
export const fromGraphQLversionsByItem = (
GQLVersionsByItem: GQLVersionedItem[],
): VersionedItem[] => {
+ if (!GQLVersionsByItem?.length) return [];
let publicIndex = -1;
const result = GQLVersionsByItem.map<VersionedItem>((version, index) => {
+ if (!version?.value) {
+ throw new Error("Invalid version data");
+ }
let status: StateType = "DRAFT";
- if (version.refs.includes("public")) {
+ if (version.refs?.includes("public")) {
status = "PUBLIC";
publicIndex = index;
}
return {
version: version.version,
status,
createdAt: version.value.createdAt,
updatedAt: version.value.updatedAt,
createdBy: { name: version.value.createdBy?.name ?? "" },
- updatedBy: { name: version.value.createdBy?.name ?? "" },
+ updatedBy: { name: version.value.updatedBy?.name ?? "" },
requests:
version.value.requests
?.filter(request =>
- request.items.some(
+ request?.items?.some(
item =>
item.item?.value.modelId === version.value.modelId &&
item.itemId === version.value.id &&
item.version === version.version,
),
)
.map(request => ({
id: request.id,
title: request.title,
})) ?? [],
};
}).map((version, index) => {
if (index > publicIndex && version.requests.length) {
version.status = "REVIEW";
}
return version;
});
return result;
};📝 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.
| export const fromGraphQLversionsByItem = ( | |
| GQLVersionsByItem: GQLVersionedItem[], | |
| ): VersionedItem[] => { | |
| let publicIndex = -1; | |
| const result = GQLVersionsByItem.map<VersionedItem>((version, index) => { | |
| let status: StateType = "DRAFT"; | |
| if (version.refs.includes("public")) { | |
| status = "PUBLIC"; | |
| publicIndex = index; | |
| } | |
| return { | |
| version: version.version, | |
| status, | |
| createdAt: version.value.createdAt, | |
| updatedAt: version.value.updatedAt, | |
| createdBy: { name: version.value.createdBy?.name ?? "" }, | |
| updatedBy: { name: version.value.createdBy?.name ?? "" }, | |
| requests: | |
| version.value.requests | |
| ?.filter(request => | |
| request.items.some( | |
| item => | |
| item.item?.value.modelId === version.value.modelId && | |
| item.itemId === version.value.id && | |
| item.version === version.version, | |
| ), | |
| ) | |
| .map(request => ({ | |
| id: request.id, | |
| title: request.title, | |
| })) ?? [], | |
| }; | |
| }).map((version, index) => { | |
| if (index > publicIndex && version.requests.length) { | |
| version.status = "REVIEW"; | |
| } | |
| return version; | |
| }); | |
| return result; | |
| }; | |
| export const fromGraphQLversionsByItem = ( | |
| GQLVersionsByItem: GQLVersionedItem[], | |
| ): VersionedItem[] => { | |
| if (!GQLVersionsByItem?.length) return []; | |
| let publicIndex = -1; | |
| const result = GQLVersionsByItem.map<VersionedItem>((version, index) => { | |
| if (!version?.value) { | |
| throw new Error("Invalid version data"); | |
| } | |
| let status: StateType = "DRAFT"; | |
| if (version.refs?.includes("public")) { | |
| status = "PUBLIC"; | |
| publicIndex = index; | |
| } | |
| return { | |
| version: version.version, | |
| status, | |
| createdAt: version.value.createdAt, | |
| updatedAt: version.value.updatedAt, | |
| createdBy: { name: version.value.createdBy?.name ?? "" }, | |
| updatedBy: { name: version.value.updatedBy?.name ?? "" }, | |
| requests: | |
| version.value.requests | |
| ?.filter(request => | |
| request?.items?.some( | |
| item => | |
| item.item?.value.modelId === version.value.modelId && | |
| item.itemId === version.value.id && | |
| item.version === version.version, | |
| ), | |
| ) | |
| .map(request => ({ | |
| id: request.id, | |
| title: request.title, | |
| })) ?? [], | |
| }; | |
| }).map((version, index) => { | |
| if (index > publicIndex && version.requests.length) { | |
| version.status = "REVIEW"; | |
| } | |
| return version; | |
| }); | |
| return result; | |
| }; |
| const { name } = isLast ? version.createdBy : version.updatedBy; | ||
| return ( | ||
| <HistoryCard key={version.version}> | ||
| <HistoryTitle onClick={() => console.log(version.version)}> |
There was a problem hiding this comment.
Implement the version click handler.
The click handler for version history is currently just logging the version. Implement the actual functionality to handle version selection.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
web/src/gql/provider.tsx (1)
43-49: LGTM! Good approach to cache management for versioned items.The composite key using
idandversionis a good choice for caching versioned items. This ensures that different versions of the same item are properly cached and managed.Consider adding a comment explaining the composite key strategy for future maintainers:
const cache = new InMemoryCache({ typePolicies: { Item: { + // Using composite key (id + version) to uniquely identify different versions of the same item keyFields: obj => `${obj.id}-${obj.version}`, }, }, });web/src/components/organisms/DataConverters/content.ts (1)
183-186: Simplify status update logic.The status update logic can be simplified by checking against the public index directly in the first map operation, avoiding the need for a second map.
Apply this diff to simplify the logic:
- }).map((version, index) => { - if (index !== publicIndex && version.requests.length) { - version.status = "REVIEW"; - } - return version; - }); + status: index !== publicIndex && version.value.requests?.some( + request => + request.state === "WAITING" && + request.items.some( + item => + item.item?.value.modelId === version.value.modelId && + item.itemId === version.value.id && + item.version === version.version, + ), + ) + ? "REVIEW" + : status,web/src/components/molecules/Content/Form/index.tsx (2)
593-595: Consider initializing activeKey for better UX.The
activeKeystate is not initialized, which might lead to an empty tab selection on initial render.Initialize the active tab to "meta" for better user experience:
- const [activeKey, setActiveKey] = useState<string>(); + const [activeKey, setActiveKey] = useState<string>("meta");
744-776: Add empty state for version history.The version history tab should display a message when no versions are available.
Add an empty state component:
{versions.length && ( <TabPane tab={t("Version History")} key="history"> <History> + {versions.length === 0 ? ( + <EmptyState + description={t("No version history available")} + icon={<Icon icon="history" />} + /> + ) : ( {versions.map((version, index) => { // ... existing code ... })} + )} </History> </TabPane> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
web/src/components/atoms/PageHeader/index.tsx(0 hunks)web/src/components/molecules/Common/Form/GroupItem/index.tsx(3 hunks)web/src/components/molecules/Common/MultiValueField/MultiValueGroup/index.tsx(3 hunks)web/src/components/molecules/Content/Details/index.tsx(7 hunks)web/src/components/molecules/Content/Form/FormItemsWrapper.tsx(1 hunks)web/src/components/molecules/Content/Form/ReferenceFormItem/index.tsx(3 hunks)web/src/components/molecules/Content/Form/fields/ComplexFieldComponents/AssetField.tsx(2 hunks)web/src/components/molecules/Content/Form/fields/ComplexFieldComponents/GroupField.tsx(4 hunks)web/src/components/molecules/Content/Form/fields/ComplexFieldComponents/ReferenceField.tsx(3 hunks)web/src/components/molecules/Content/Form/index.tsx(14 hunks)web/src/components/molecules/Content/types.ts(2 hunks)web/src/components/molecules/Integration/IntegrationTable/index.tsx(1 hunks)web/src/components/molecules/Member/MemberTable/index.tsx(1 hunks)web/src/components/molecules/Request/Details/ItemForm.tsx(3 hunks)web/src/components/molecules/Request/Details/Request.tsx(1 hunks)web/src/components/molecules/Schema/index.tsx(1 hunks)web/src/components/organisms/DataConverters/content.ts(3 hunks)web/src/components/organisms/Project/Content/ContentDetails/hooks.ts(10 hunks)web/src/components/organisms/Project/Content/ContentDetails/index.tsx(4 hunks)web/src/gql/graphql-client-api.tsx(2 hunks)web/src/gql/provider.tsx(1 hunks)web/src/gql/queries/item.ts(1 hunks)
💤 Files with no reviewable changes (1)
- web/src/components/atoms/PageHeader/index.tsx
✅ Files skipped from review due to trivial changes (4)
- web/src/components/molecules/Schema/index.tsx
- web/src/components/molecules/Request/Details/Request.tsx
- web/src/components/molecules/Member/MemberTable/index.tsx
- web/src/components/molecules/Integration/IntegrationTable/index.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/gql/queries/item.ts
- web/src/components/organisms/Project/Content/ContentDetails/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: ci-web / ci
- GitHub Check: Redirect rules - reearth-cms
- GitHub Check: Header rules - reearth-cms
- GitHub Check: Pages changed - reearth-cms
🔇 Additional comments (18)
web/src/components/molecules/Content/types.ts (2)
10-10: LGTM! Clear type definition for form values.The
FormValuestype correctly represents the structure of form data, allowing for both simple values and nested group values.
99-105: LGTM! Well-structured type for version history.The
VersionedItemtype includes all necessary fields for tracking version history:
- version identifier
- status tracking
- timestamp for version creation
- creator information
- associated requests
web/src/components/molecules/Request/Details/ItemForm.tsx (2)
16-16: LGTM! Good improvement to type safety.Making
referencedItemsa required prop ensures that the component always has the necessary data to function properly.
42-43: LGTM! Consistent prop naming across components.The standardization of the
referencedItemsprop name acrossReferenceFieldandGroupFieldcomponents improves code consistency and maintainability.Also applies to: 52-53
web/src/components/molecules/Content/Form/fields/ComplexFieldComponents/ReferenceField.tsx (1)
10-11: LGTM! Clean prop structure update.The replacement of
formItemsDatawithreferencedItemsmaintains consistency with other components while preserving the component's functionality.web/src/components/molecules/Content/Form/FormItemsWrapper.tsx (1)
1-127: LGTM! Well-structured component with good separation of concerns.The component is well-organized, handles different field types appropriately, and follows React best practices. The props interface is comprehensive and the styled components provide good layout management.
web/src/components/molecules/Content/Form/ReferenceFormItem/index.tsx (1)
15-15: LGTM! Consistent prop naming across components.The transition from
formItemsDatatoreferencedItemsaligns with similar changes in related components, maintaining consistency throughout the codebase.Also applies to: 81-85
web/src/components/molecules/Content/Form/fields/ComplexFieldComponents/AssetField.tsx (1)
16-17: LGTM! Type-safe property additions.The addition of
onGetAssetproperty and repositioning ofdisabledproperty maintain type safety while enhancing the component's functionality.web/src/components/organisms/DataConverters/content.ts (1)
150-190: Add error handling for undefined or null values.The function should handle potential undefined or null values from the GraphQL response to prevent runtime errors.
web/src/components/molecules/Content/Form/fields/ComplexFieldComponents/GroupField.tsx (1)
18-18: LGTM! Consistent prop renaming.The renaming of
formItemsDatatoreferencedItemsis consistent across the component and its children. The prop is now required, which helps ensure data consistency.Also applies to: 64-64, 112-112, 153-153
web/src/components/molecules/Common/MultiValueField/MultiValueGroup/index.tsx (1)
29-29: LGTM! Consistent prop renaming.The renaming of
formItemsDatatoreferencedItemsis consistent across the component and its children. The prop is now required, which helps ensure data consistency.Also applies to: 78-78, 195-195
web/src/components/molecules/Common/Form/GroupItem/index.tsx (1)
25-25: LGTM! Consistent prop renaming.The renaming of
formItemsDatatoreferencedItemsis consistent across the component and its children. The prop is now required, which helps ensure data consistency.Also applies to: 77-77, 220-220
web/src/components/molecules/Content/Details/index.tsx (1)
9-15: LGTM! Well-structured version history support.The changes effectively introduce version history support:
- New types
VersionedItemandFormValuesare imported.- New props
versionsandonGetVersionedItemare added.- Props are correctly passed down to
ContentForm.The async nature of
onGetVersionedItemis appropriate for fetching versioned items.Also applies to: 40-40, 73-73, 137-137, 174-174, 253-253, 265-265
web/src/components/organisms/Project/Content/ContentDetails/hooks.ts (2)
596-600: Verify error handling in version query.The version query implementation should handle potential errors to ensure robust version management.
Add error handling to prevent potential issues:
const { data: versionsData } = useVersionsByItemQuery({ fetchPolicy: "cache-and-network", variables: { itemId: itemId ?? "" }, skip: !itemId, + onError: error => { + Notification.error({ message: t("Failed to fetch versions.") }); + console.error("Version query error:", error); + }, });
299-301: LGTM! Proper refetch queries configuration.The mutation handlers correctly include "VersionsByItem" in their refetch queries to ensure version data stays in sync.
Also applies to: 520-522
web/src/gql/graphql-client-api.tsx (3)
2573-2578: LGTM! New version query variables type added.The new
VersionsByItemQueryVariablestype is correctly defined with the requireditemIdparameter of typeID.
2827-2871: LGTM! New version query definition added.The
VersionsByItemQueryis well-structured and includes all necessary fields for version history:
- Version string
- References array
- Item details including ID, version, model ID, status, timestamps
- Creator and updater information
- Associated requests
4890-4905: LGTM! Query hooks generated correctly.The Apollo hooks for the versions query are properly generated:
useVersionsByItemQueryfor standard queryuseVersionsByItemLazyQueryfor lazy loadinguseVersionsByItemSuspenseQueryfor suspense support
| const handleGetVersionedItem = useCallback( | ||
| async (id: string, version: string) => { | ||
| const res = await getItem({ | ||
| variables: { | ||
| id, | ||
| // version, | ||
| }, | ||
| }); | ||
| return await initialValueGet(fromGraphQLItem(res.data?.node as GQLItem)); | ||
| }, | ||
| [getItem, initialValueGet], | ||
| ); |
There was a problem hiding this comment.
Fix commented out version parameter in handleGetVersionedItem.
The version parameter is commented out in the getItem query, which means version history functionality might not work as expected.
Uncomment the version parameter to ensure proper version retrieval:
const handleGetVersionedItem = useCallback(
async (id: string, version: string) => {
const res = await getItem({
variables: {
id,
- // version,
+ version,
},
});
return await initialValueGet(fromGraphQLItem(res.data?.node as GQLItem));
},
[getItem, initialValueGet],
);📝 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 handleGetVersionedItem = useCallback( | |
| async (id: string, version: string) => { | |
| const res = await getItem({ | |
| variables: { | |
| id, | |
| // version, | |
| }, | |
| }); | |
| return await initialValueGet(fromGraphQLItem(res.data?.node as GQLItem)); | |
| }, | |
| [getItem, initialValueGet], | |
| ); | |
| const handleGetVersionedItem = useCallback( | |
| async (id: string, version: string) => { | |
| const res = await getItem({ | |
| variables: { | |
| id, | |
| version, | |
| }, | |
| }); | |
| return await initialValueGet(fromGraphQLItem(res.data?.node as GQLItem)); | |
| }, | |
| [getItem, initialValueGet], | |
| ); |
| const handleRestore = useCallback(() => { | ||
| form.setFieldsValue(versionForm.getFieldsValue()); | ||
| }, [form, versionForm]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add confirmation dialog for version restoration.
The version restoration process should prompt for confirmation to prevent accidental data loss.
Add a confirmation dialog:
const handleRestore = useCallback(() => {
+ Modal.confirm({
+ title: t("Restore version"),
+ content: t("Are you sure you want to restore this version? This will overwrite your current changes."),
+ onOk: () => {
form.setFieldsValue(versionForm.getFieldsValue());
+ },
+ });
}, [form, versionForm]);📝 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 handleRestore = useCallback(() => { | |
| form.setFieldsValue(versionForm.getFieldsValue()); | |
| }, [form, versionForm]); | |
| const handleRestore = useCallback(() => { | |
| Modal.confirm({ | |
| title: t("Restore version"), | |
| content: t("Are you sure you want to restore this version? This will overwrite your current changes."), | |
| onOk: () => { | |
| form.setFieldsValue(versionForm.getFieldsValue()); | |
| }, | |
| }); | |
| }, [form, versionForm]); |
| const versionClick = useCallback( | ||
| async (versionedItem: VersionedItem) => { | ||
| if (versionedItem.version === item?.version || !item?.id) return; | ||
| const res = await onGetVersionedItem(item.id, versionedItem.version); | ||
| versionForm.setFieldsValue(res); | ||
| setVersionedItem(versionedItem); | ||
| }, | ||
| [item?.id, item?.version, onGetVersionedItem, versionForm], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add loading state for version retrieval.
The version click handler should show a loading state while fetching the versioned item data.
Add loading state to improve user feedback:
+ const [versionLoading, setVersionLoading] = useState(false);
const versionClick = useCallback(
async (versionedItem: VersionedItem) => {
if (versionedItem.version === item?.version || !item?.id) return;
+ setVersionLoading(true);
try {
const res = await onGetVersionedItem(item.id, versionedItem.version);
versionForm.setFieldsValue(res);
setVersionedItem(versionedItem);
+ } catch (error) {
+ Notification.error({ message: t("Failed to fetch version.") });
+ } finally {
+ setVersionLoading(false);
+ }
},
[item?.id, item?.version, onGetVersionedItem, versionForm],
);📝 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 versionClick = useCallback( | |
| async (versionedItem: VersionedItem) => { | |
| if (versionedItem.version === item?.version || !item?.id) return; | |
| const res = await onGetVersionedItem(item.id, versionedItem.version); | |
| versionForm.setFieldsValue(res); | |
| setVersionedItem(versionedItem); | |
| }, | |
| [item?.id, item?.version, onGetVersionedItem, versionForm], | |
| ); | |
| const [versionLoading, setVersionLoading] = useState(false); | |
| const versionClick = useCallback( | |
| async (versionedItem: VersionedItem) => { | |
| if (versionedItem.version === item?.version || !item?.id) return; | |
| setVersionLoading(true); | |
| try { | |
| const res = await onGetVersionedItem(item.id, versionedItem.version); | |
| versionForm.setFieldsValue(res); | |
| setVersionedItem(versionedItem); | |
| } catch (error) { | |
| Notification.error({ message: t("Failed to fetch version.") }); | |
| } finally { | |
| setVersionLoading(false); | |
| } | |
| }, | |
| [item?.id, item?.version, onGetVersionedItem, versionForm], | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
web/src/components/organisms/DataConverters/content.ts (1)
150-199:⚠️ Potential issueAdd error handling for undefined or null values.
The function should handle potential undefined or null values from the GraphQL response to prevent runtime errors.
Apply this diff to add error handling:
export const fromGraphQLversionsByItem = ( GQLVersionsByItem: GQLVersionedItem[], ): VersionedItem[] => { + if (!GQLVersionsByItem?.length) return []; let publicIndex = -1; const result = GQLVersionsByItem.map<VersionedItem>((version, index) => { + if (!version?.value) { + throw new Error("Invalid version data"); + } let status: StateType = "DRAFT"; - if (version.refs.includes("public")) { + if (version.refs?.includes("public")) { status = "PUBLIC"; publicIndex = index; } return { version: version.version, status, timestamp: version.value.updatedAt ?? version.value.createdAt, creator: { name: version.value.updatedBy?.name ?? version.value.createdBy?.name ?? "" }, fields: version.value.fields.map( field => ({ schemaFieldId: field.schemaFieldId, itemGroupId: field.itemGroupId, type: field.type, value: field.value, }) as ItemField, ), requests: version.value.requests ?.filter( request => + request?.state === "WAITING" && request?.items?.some( item => item.item?.value.modelId === version.value.modelId && item.itemId === version.value.id && item.version === version.version, ), ) .map(request => ({ id: request.id, title: request.title, })) ?? [], }; }); return result; };web/src/components/molecules/Content/Form/index.tsx (2)
610-612: 🛠️ Refactor suggestionAdd confirmation dialog for version restoration.
The restore operation should prompt for confirmation as it overwrites the current form state.
const handleRestore = useCallback(() => { + Modal.confirm({ + title: t("Restore version"), + content: t("Are you sure you want to restore this version? This will overwrite your current changes."), + onOk: () => { form.setFieldsValue(versionForm.getFieldsValue()); + }, + }); }, [form, versionForm]);
596-604: 🛠️ Refactor suggestionAdd error handling and loading state to version management.
The version click handler should handle errors and show loading state while fetching version data.
+ const [versionLoading, setVersionLoading] = useState(false); const versionClick = useCallback( async (versionedItem: VersionedItem) => { if (versionedItem.version === item?.version || !item?.id) return; + setVersionLoading(true); try { const res = await onGetVersionedItem(versionedItem.version); versionForm.setFieldsValue(res); setVersionedItem(versionedItem); + } catch (error) { + Notification.error({ message: t("Failed to fetch version data") }); + } finally { + setVersionLoading(false); + } }, [item?.id, item?.version, onGetVersionedItem, versionForm], );
🧹 Nitpick comments (3)
web/src/components/molecules/Content/Form/index.tsx (3)
65-65: Add type validation for version management props.Consider adding validation for the version parameter in
onGetVersionedItemto ensure it's a valid version string.- versions: VersionedItem[]; - onGetVersionedItem: (version: string) => Promise<FormValues>; + versions: VersionedItem[]; + onGetVersionedItem: (version: NonEmptyString) => Promise<FormValues>; + type NonEmptyString = string & { __brand: 'NonEmptyString' };Also applies to: 118-118
593-595: Initialize the active tab state.The
activeKeystate is not initialized, which could lead to undefined behavior. Consider setting a default tab.- const [activeKey, setActiveKey] = useState<string>(); + const [activeKey, setActiveKey] = useState<string>("meta");
750-755: Improve accessibility of version history items.Add ARIA labels and keyboard navigation support for better accessibility.
- <HistoryTitle onClick={() => versionClick(version)}> + <HistoryTitle + onClick={() => versionClick(version)} + onKeyPress={(e) => e.key === 'Enter' && versionClick(version)} + role="button" + tabIndex={0} + aria-label={t("View version from {{date}}", { date: dateTimeFormat(version.timestamp, "YYYY/MM/DD, HH:mm") })}> <Tooltip title={t(version.status)}> <Badge color={stateColors[version.status]} /> </Tooltip> {dateTimeFormat(version.timestamp, "YYYY/MM/DD, HH:mm")} </HistoryTitle>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
web/src/components/molecules/Content/Details/index.tsx(7 hunks)web/src/components/molecules/Content/Form/index.tsx(14 hunks)web/src/components/molecules/Content/types.ts(2 hunks)web/src/components/organisms/DataConverters/content.ts(3 hunks)web/src/components/organisms/Project/Content/ContentDetails/hooks.ts(10 hunks)web/src/gql/graphql-client-api.tsx(2 hunks)web/src/gql/queries/item.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/gql/queries/item.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-web / ci
🔇 Additional comments (12)
web/src/components/molecules/Content/types.ts (2)
10-10: LGTM! Type definition enhances form data type safety.The
FormValuestype provides a clear and type-safe way to handle form data by leveraging existing types.
99-106: LGTM! Well-structured version history type.The
VersionedItemtype effectively captures version metadata and content with:
- Version identifier and status tracking
- Timestamp for version history
- Creator information
- Fields and associated requests
web/src/components/organisms/DataConverters/content.ts (1)
299-299: LGTM! Proper query refetching.The addition of "VersionsByItem" to refetchQueries ensures version data stays in sync after mutations.
Also applies to: 520-520
web/src/components/molecules/Content/Details/index.tsx (2)
40-40: LGTM! Well-typed version history props.The new props enhance the component with version history capabilities:
versionsarray for storing version historyonGetVersionedItemcallback for retrieving specific versionsAlso applies to: 73-73
137-137: LGTM! Proper prop forwarding.The version-related props are correctly passed down to the
ContentFormcomponent, maintaining the data flow.Also applies to: 174-174, 253-253, 265-265
web/src/components/organisms/Project/Content/ContentDetails/hooks.ts (3)
595-599: LGTM! Well-configured version query.The version query is properly configured with:
- Cache-and-network fetch policy for optimal data freshness
- Skip condition to prevent unnecessary queries
601-607: LGTM! Efficient version data processing.The version data processing is optimized with:
- Memoization to prevent unnecessary recalculations
- Reverse sorting for chronological display
417-463: LGTM! Well-refactored initialization logic.The
initialValueGetfunction is well-structured with:
- Async/await for better error handling
- Support for both new and existing items
- Clear separation of group and field handling
web/src/components/molecules/Content/Form/index.tsx (1)
1-920: Well-structured implementation of version history feature!The implementation effectively manages version history with proper separation of concerns, state management, and UI components. The code is clean and maintainable.
web/src/gql/graphql-client-api.tsx (3)
2573-2578: LGTM! Query variables type is well-defined.The
VersionsByItemQueryVariablestype properly enforces the requireditemIdparameter with correct typing.
4827-4877: LGTM! Query structure is comprehensive and well-typed.The
VersionsByItemquery is well-structured with:
- Complete version metadata (version string, refs)
- Nested item data with all relevant fields
- Associated request information
- Proper field selections for the version history UI
4896-4911: LGTM! Generated hooks follow Apollo best practices.The generated hooks provide:
- Standard query hook with loading/error states
- Lazy query variant for manual execution
- Suspense query variant for React Suspense support
- Proper TypeScript type safety throughout
| const handleGetVersionedItem = useCallback( | ||
| (version: string) => { | ||
| const res = versions.find(v => v.version === version); | ||
| if (res) { | ||
| return initialValueGet(res.fields); | ||
| } | ||
| }, | ||
| [initialValueGet, versions], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for version not found.
The handleGetVersionedItem function should handle the case when a version is not found.
Apply this diff to add error handling:
const handleGetVersionedItem = useCallback(
(version: string) => {
const res = versions.find(v => v.version === version);
if (res) {
return initialValueGet(res.fields);
+ } else {
+ throw new Error(`Version ${version} not found`);
}
},
[initialValueGet, versions],
);📝 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 handleGetVersionedItem = useCallback( | |
| (version: string) => { | |
| const res = versions.find(v => v.version === version); | |
| if (res) { | |
| return initialValueGet(res.fields); | |
| } | |
| }, | |
| [initialValueGet, versions], | |
| ); | |
| const handleGetVersionedItem = useCallback( | |
| (version: string) => { | |
| const res = versions.find(v => v.version === version); | |
| if (res) { | |
| return initialValueGet(res.fields); | |
| } else { | |
| throw new Error(`Version ${version} not found`); | |
| } | |
| }, | |
| [initialValueGet, versions] | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
web/src/components/molecules/Content/Form/index.tsx (2)
611-615: 🛠️ Refactor suggestionAdd confirmation dialog for version restoration.
Version restoration should prompt for confirmation to prevent accidental data loss.
const handleRestore = useCallback(() => { + Modal.confirm({ + title: t("Restore version"), + content: t("Are you sure you want to restore this version? This will overwrite your current changes."), + onOk: () => { const values = versionForm.getFieldsValue(); form.setFieldsValue(values); handleValuesChange(values); + }, + }); }, [form, handleValuesChange, versionForm, t]);
595-605: 🛠️ Refactor suggestionAdd loading state and error handling to version click handler.
The version click handler should provide feedback during loading and handle errors gracefully.
const [activeKey, setActiveKey] = useState<string>(); const [versionedItem, setVersionedItem] = useState<VersionedItem>(); + const [versionLoading, setVersionLoading] = useState(false); const versionClick = useCallback( async (versionedItem: VersionedItem) => { if (versionedItem.version === item?.version) return; + setVersionLoading(true); try { const res = await onGetVersionedItem(versionedItem.version); versionForm.setFieldsValue(res); setVersionedItem(versionedItem); + } catch (error) { + Notification.error({ message: t("Failed to fetch version") }); + } finally { + setVersionLoading(false); + } }, [onGetVersionedItem, versionForm, item?.version, t], );
🧹 Nitpick comments (4)
web/src/components/organisms/Project/Content/ContentDetails/hooks.ts (3)
418-464: Consider memoizing groupInitialValuesUpdate.The
groupInitialValuesUpdatefunction is recreated on every render since it's defined insideinitialValueGet. Consider extracting and memoizing it to optimize performance.+const groupInitialValuesUpdate = useCallback((group: Group, itemGroupId: string, initialValues: FormValues) => { + group?.schema?.fields?.forEach(field => { + initialValues[field.id] = { + ...(initialValues[field.id] as FormGroupValue), + ...{ [itemGroupId]: valueGet(field) }, + }; + }); +}, [valueGet]); const initialValueGet = useCallback( async (fields?: ItemField[]) => { const initialValues: FormValues = {}; - const groupInitialValuesUpdate = (group: Group, itemGroupId: string) => { - group?.schema?.fields?.forEach(field => { - initialValues[field.id] = { - ...(initialValues[field.id] as FormGroupValue), - ...{ [itemGroupId]: valueGet(field) }, - }; - }); - };
596-608: Consider adding loading state for versions.The versions query doesn't expose its loading state, which could lead to UI inconsistencies during data fetching.
-const { data: versionsData } = useVersionsByItemQuery({ +const { data: versionsData, loading: versionsLoading } = useVersionsByItemQuery({ fetchPolicy: "cache-and-network", variables: { itemId: itemId ?? "" }, skip: !itemId, }); const versions = useMemo( () => versionsData ? fromGraphQLversionsByItem(versionsData.versionsByItem as GQLVersionedItem[]).reverse() : [], [versionsData], ); +// Add versionsLoading to the return object return { // ...other properties versions, + versionsLoading, };
466-472: Consider adding error boundary for initialValuesSet.The
handleInitialValuesSetfunction could potentially throw ifinitialValueGetfails, but there's no error handling.useEffect(() => { if (itemLoading) return; const handleInitialValuesSet = async () => { + try { setInitialFormValues(await initialValueGet(currentItem?.fields ?? undefined)); + } catch (error) { + console.error('Failed to set initial values:', error); + // Consider showing an error notification to the user + Notification.error({ message: t("Failed to load item data.") }); + } }; handleInitialValuesSet(); }, [currentItem, initialValueGet, itemLoading]);web/src/components/molecules/Content/Form/index.tsx (1)
747-779: Enhance accessibility of version history UI.The version history UI is well-structured but could benefit from accessibility improvements.
<TabPane tab={t("Version History")} key="history"> <History> {versions.map((version, index) => { return ( - <HistoryCard key={version.version}> + <HistoryCard + key={version.version} + role="listitem" + aria-label={t("Version from {{date}}", { date: dateTimeFormat(version.timestamp, "YYYY/MM/DD, HH:mm") })} + > - <HistoryTitle onClick={() => versionClick(version)}> + <HistoryTitle + onClick={() => versionClick(version)} + role="button" + tabIndex={0} + onKeyPress={(e) => e.key === 'Enter' && versionClick(version)} + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/src/components/molecules/Content/Form/index.tsx(15 hunks)web/src/components/organisms/Project/Content/ContentDetails/hooks.ts(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: ci-web / ci
- GitHub Check: Redirect rules - reearth-cms
- GitHub Check: Header rules - reearth-cms
- GitHub Check: Pages changed - reearth-cms
🔇 Additional comments (6)
web/src/components/organisms/Project/Content/ContentDetails/hooks.ts (3)
7-7: LGTM! Clean import organization.The new imports are well-organized and properly grouped with related imports.
Also applies to: 20-22, 30-30, 44-44
299-301: LGTM! Proper query refetching.The mutation correctly refetches both "GetItem" and "VersionsByItem" queries to ensure UI consistency after updates.
610-616: Add error handling for version not found.The
handleGetVersionedItemfunction should handle the case when a version is not found.web/src/components/molecules/Content/Form/index.tsx (3)
6-16: LGTM: Props and imports are well-structured.The new imports and props are properly organized and align well with the version history feature requirements.
Also applies to: 65-65, 118-118
277-299: LGTM: Form change handling is robust.The implementation properly handles form changes, including complex group field comparisons and empty value conversions.
822-921: LGTM: Styled components are well-structured.The styling implementation is clean, modular, and follows best practices for component organization.
Overview
What I've done
What I haven't done
How I tested
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
FormItemsWrappercomponent for dynamic rendering of various field types.Refactor
referencedItems.Style
PageHeaderacross multiple components to ensure a consistent white background.