add a primary asset cover image to library pages #224
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request introduce a new field, Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Outside diff range and nitpick comments (34)
web/src/screens/media/MediaEditModal.tsx (2)
1-1: Correct the import path casing for consistency.The import path for
ModalDrawerhas inconsistent casing. It should beModalDrawerinstead ofModaldrawerto match typical React component naming conventions.Apply this change:
-import { ModalDrawer } from "src/components/site/Modaldrawer/Modaldrawer"; +import { ModalDrawer } from "src/components/site/ModalDrawer/ModalDrawer";
6-14: Approve component structure with a minor optimization.The
MediaEditModalcomponent is well-structured and correctly implements the modal functionality. Props are appropriately passed down to child components.Consider removing the unnecessary fragment (
<>) as there's only one child element:export function MediaEditModal(props: UseDisclosureProps & Props) { return ( - <> <ModalDrawer isOpen={props.isOpen} onClose={props.onClose} title="Upload"> <MediaEditScreen {...props} /> </ModalDrawer> - </> ); }web/src/screens/media/MediaEditScreen.tsx (1)
1-5: Remove unused import.The
FileUploadFileAcceptDetailsimport from "@ark-ui/react" is not used in the current implementation. Consider removing it to keep the imports clean and avoid potential confusion.-import { FileUploadFileAcceptDetails } from "@ark-ui/react"; import { Asset } from "@/api/openapi-schema"; import { ImageEditor } from "@/components/site/ImageEditor/ImageEditor"; import { LStack } from "@/styled-system/jsx";web/src/components/library/LibraryPageCoverImageControl/useLibraryPageCoverImageControl.ts (1)
1-43: Overall, the useLibraryPageCoverImageControl hook is well-implemented and aligns with the PR objectives.The hook effectively manages cover images for library pages, providing clear and concise functionality for uploading and removing images. It follows React best practices, includes proper error handling, and ensures data consistency through revalidation. The code is clean, readable, and maintainable.
One minor suggestion for improvement:
Consider adding JSDoc comments to the hook and its returned functions to provide better documentation for consumers of this hook.
web/src/components/library/LibraryPageCoverImageControl/LibraryPageCoverImageControl.tsx (3)
11-18: Component structure looks good, consider addressing the TODO.The component structure follows React best practices, utilizing hooks and conditional rendering effectively. The separation of logic into a custom hook is commendable.
However, there's a TODO comment about grouping options in the editing toolbar. Consider creating an issue to track this enhancement for future implementation.
Would you like me to create a GitHub issue to track the TODO for grouping editing toolbar options?
19-37: Functionality aligns with PR objectives, consider adding a loading state.The component successfully implements the required functionality for managing cover images, allowing both removal and upload/update operations. The conditional rendering based on the existence of a cover image is well-implemented.
To enhance user experience, consider adding a loading state during image upload or removal operations. This could prevent users from triggering multiple actions while an operation is in progress.
Here's a suggestion to add a loading state:
export function LibraryPageCoverImageControl(props: Props) { const { hasCoverImage, + isLoading, handlers: { handleUploadCoverImage, handleRemoveCoverImage }, } = useLibraryPageCoverImageControl(props); return ( <> {hasCoverImage && ( <Button type="button" size="xs" variant="outline" onClick={handleRemoveCoverImage} + disabled={isLoading} > - <ImageMinusIcon /> + {isLoading ? <Spinner size="sm" /> : <ImageMinusIcon />} remove cover </Button> )} <AssetUploadAction operation={hasCoverImage ? "update" : "add"} onFinish={handleUploadCoverImage} accept={["image/png", "image/jpeg", "image/gif"]} + disabled={isLoading} /> </> ); }Note: You'll need to implement the
isLoadingstate in theuseLibraryPageCoverImageControlhook and add aSpinnercomponent to your UI library.
19-37: Enhance accessibility with ARIA labels.While the component uses standard UI components which likely have built-in accessibility features, consider adding ARIA labels to improve the experience for screen reader users.
Here's a suggestion to enhance accessibility:
{hasCoverImage && ( <Button type="button" size="xs" variant="outline" onClick={handleRemoveCoverImage} + aria-label="Remove cover image" > <ImageMinusIcon /> remove cover </Button> )} <AssetUploadAction operation={hasCoverImage ? "update" : "add"} onFinish={handleUploadCoverImage} accept={["image/png", "image/jpeg", "image/gif"]} + aria-label={hasCoverImage ? "Update cover image" : "Add cover image"} />web/src/api/openapi-schema/nodeMutableProps.ts (1)
30-30: LGTM: New property added correctly. Consider updating interface documentation.The new
primary_image_asset_idproperty is correctly added to theNodeMutablePropsinterface. It's optional and uses theIdentifiertype as expected.Consider updating the interface documentation comment to mention this new property, especially since it's a significant addition related to the primary image feature.
internal/ent/schema/node.go (2)
27-27: LGTM! Consider adding a comment for clarity.The addition of the
primary_asset_idfield is well-implemented. It's consistent with other ID fields in the schema and correctly set as optional and nillable.Consider adding a comment to explain the purpose of this field, similar to other fields in the schema. For example:
- field.String("primary_asset_id").GoType(xid.ID{}).Optional().Nillable(), + field.String("primary_asset_id").GoType(xid.ID{}).Optional().Nillable(). + Comment("The ID of the primary asset (cover image) associated with this node"),
54-57: LGTM! Consider adding a comment for consistency.The addition of the
primary_imageedge is well-implemented. It correctly corresponds to the newprimary_asset_idfield and is appropriately set as Unique.For consistency with other edges in the schema, consider adding a comment to explain the purpose of this edge. For example:
edge.To("primary_image", Asset.Type). Field("primary_asset_id"). - Unique(), + Unique(). + Comment("The primary asset (cover image) associated with this node"),web/src/recipes/file-upload.ts (1)
68-73: Consider addingborderRadiusto itemPreviewImageThe
itemPreviewImagestyle could be enhanced by adding aborderRadiusproperty. This would ensure that the preview image has rounded corners, which is often desirable for a polished look and consistency with other UI elements.Consider adding the following property to the
itemPreviewImagestyle:itemPreviewImage: { aspectRatio: "1", height: "10", objectFit: "scale-down", width: "10", + borderRadius: "md", },app/resources/library/mapping.go (1)
66-79: Approved with a minor suggestionThe modified return statement correctly includes the new
PrimaryImagefield in theNodestruct initialization. The overall structure and indentation are consistent with the rest of the code.However, to improve readability and maintain consistency, consider grouping related fields together. For example, you might want to place
PrimaryImagenext toAssetssince they are both related to visual content.Here's a suggested reordering of the fields:
return &Node{ Mark: NewMark(c.ID, c.Slug), CreatedAt: c.CreatedAt, UpdatedAt: c.UpdatedAt, Name: c.Name, Description: opt.NewPtr(c.Description), Content: richContent, Assets: assets, PrimaryImage: primaryImage, WebLink: link, Owner: *pro, Parent: parent, Nodes: nodes, Visibility: visibility, Metadata: c.Metadata, }, nilinternal/ent/er.html (2)
262-262: LGTM: Relationship correctly implements primary image associationThe new relationship between
NodeandAssetcorrectly implements the association for the primary image. The relationship type (}o--o|) accurately represents that a Node can have zero or one primary image Asset.Consider enhancing the relationship description for clarity:
-Node }o--o| Asset : "primary_image" +Node }o--o| Asset : "primary_image (cover)"This small change would make it immediately clear that this relationship represents the cover image mentioned in the PR objectives.
165-165: Consider handling of original and cropped cover imagesThe changes to the
Nodeentity and its relationship withAsseteffectively implement the primary cover image feature for library pages. The data model modifications are minimal and focused, which is good for maintainability.However, the PR objectives mention a need to handle both original and cropped versions of the cover image. The current data model doesn't explicitly address this requirement. Consider the following options:
- Use the existing
Assetentity'smetadatafield to store information about the cropped version.- Create a separate
CroppedAssetentity that references the originalAsset.- Add fields to the
Assetentity to store cropping information.Each approach has its trade-offs in terms of complexity and query performance. Please clarify how you intend to handle the original and cropped versions of the cover image in the data model.
Also applies to: 262-262
app/services/library/node_mutate/node.go (1)
Missing changes in Create and Update methods
The code diff does not show the expected modifications to the
CreateandUpdatemethods. Please ensure that all intended changes, especially related to the newPrimaryImagefield, are included in the diff.🔗 Analysis chain
Line range hint
1-1: Request for information: Changes to Create and Update methodsThe AI summary mentioned modifications to the
CreateandUpdatemethods, but these changes are not visible in the provided code diff. Could you please provide the specific changes made to these methods? This information is crucial for a comprehensive review of the implementation of the newPrimaryImagefield.To help identify the changes, you can run the following command:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find changes in Create and Update methods rg -U 'func \(s \*service\) (Create|Update)' app/services/library/node_mutate/node.go -A 20 -B 5Length of output: 1478
app/transports/http/bindings/nodes.go (3)
90-91: LGTM: Primary image handling addedThe addition of
PrimaryImagehandling is consistent with the PR objectives. Good use ofopt.Mapandopt.NewPtrfor optional value handling.Consider adding a comment explaining the purpose of this new field for better code readability.
235-236: LGTM: Primary image handling added to node updateThe addition of
PrimaryImagehandling in theNodeUpdatefunction is consistent with the changes inNodeCreate. This allows for proper modification of the primary image during updates.For consistency, consider grouping the
PrimaryImagefield with other similar fields in thenode_mutate.Partialstruct initialization.Also applies to: 244-244
350-360: LGTM: Primary image added to node serializationThe addition of the
PrimaryImagefield to bothserialiseNodeandserialiseNodeWithItemsfunctions ensures that the primary image is included in the serialized output of nodes. The use ofopt.Mapand.Ptr()is consistent with the handling of other optional fields.Consider grouping similar fields together in the struct initialization for better code organization. For example, you could place
PrimaryImagenext toAssetsorContent.Also applies to: 372-382
internal/ent/mutation.go (5)
14494-14495: LGTM with a minor suggestion.The addition of
primary_imageandclearedprimary_imagefields aligns with the PR objective. However, consider using camelCase for field names to maintain consistency with Go naming conventions.Consider renaming:
- primary_image *xid.ID - clearedprimary_image bool + primaryImage *xid.ID + clearedPrimaryImage bool
14995-15042: LGTM with suggestions for improvement.The new methods for managing the primary asset ID are well-implemented and follow the existing patterns in the codebase. However, there are a few areas for improvement:
- Naming Convention: Some method names use snake_case instead of the Go-standard camelCase.
- Error Handling: The
OldPrimaryAssetIDmethod could benefit from more specific error types.Consider the following improvements:
- Rename methods to follow Go conventions:
-func (m *NodeMutation) SetPrimaryAssetID(x xid.ID) { +func (m *NodeMutation) SetPrimaryAssetID(x xid.ID) { -func (m *NodeMutation) PrimaryAssetID() (r xid.ID, exists bool) { +func (m *NodeMutation) PrimaryAssetID() (r xid.ID, exists bool) { -func (m *NodeMutation) OldPrimaryAssetID(ctx context.Context) (v *xid.ID, err error) { +func (m *NodeMutation) OldPrimaryAssetID(ctx context.Context) (v *xid.ID, err error) {
- Consider using custom error types for more specific error handling in
OldPrimaryAssetID:var ( ErrNotUpdateOperation = errors.New("operation is not UpdateOne") ErrMissingIDField = errors.New("missing ID field in the mutation") ) func (m *NodeMutation) OldPrimaryAssetID(ctx context.Context) (v *xid.ID, err error) { if !m.op.Is(OpUpdateOne) { return v, ErrNotUpdateOperation } if m.id == nil || m.oldValue == nil { return v, ErrMissingIDField } // ... rest of the method }
15312-15350: LGTM with minor naming convention suggestions.The new methods for managing the "primary_image" edge are well-implemented and follow the existing patterns in the codebase. The comment explaining the purpose of
PrimaryImageIDsis particularly helpful.Consider renaming some methods to follow Go's camelCase convention:
-func (m *NodeMutation) SetPrimaryImageID(id xid.ID) { +func (m *NodeMutation) SetPrimaryImageID(id xid.ID) { -func (m *NodeMutation) ClearPrimaryImage() { +func (m *NodeMutation) ClearPrimaryImage() { -func (m *NodeMutation) PrimaryImageCleared() bool { +func (m *NodeMutation) PrimaryImageCleared() bool { -func (m *NodeMutation) PrimaryImageID() (id xid.ID, exists bool) { +func (m *NodeMutation) PrimaryImageID() (id xid.ID, exists bool) { -func (m *NodeMutation) PrimaryImageIDs() (ids []xid.ID) { +func (m *NodeMutation) PrimaryImageIDs() (ids []xid.ID) { -func (m *NodeMutation) ResetPrimaryImage() { +func (m *NodeMutation) ResetPrimaryImage() {
16133-16135: LGTM with a minor suggestion.The new condition to check if the primary image edge has been cleared is implemented correctly. It follows the existing pattern of checking a boolean flag and appending the edge to the
edgesslice if cleared.Consider renaming the flag to follow Go's camelCase convention:
- if m.clearedprimary_image { - edges = append(edges, node.EdgePrimaryImage) - } + if m.clearedPrimaryImage { + edges = append(edges, node.EdgePrimaryImage) + }
16164-16165: LGTM with a minor suggestion.The new case for the primary image edge in the
EdgeClearedmethod is implemented correctly. It follows the existing pattern of returning the corresponding cleared flag.Consider renaming the flag to follow Go's camelCase convention:
- case node.EdgePrimaryImage: - return m.clearedprimary_image + case node.EdgePrimaryImage: + return m.clearedPrimaryImageapp/transports/http/openapi/server_gen.go (1)
1524-1526: LGTM: Addition of PrimaryImageAssetId field with a minor suggestionThe addition of the
PrimaryImageAssetIdfield is consistent with the PR's objective. The use of*AssetIDallows for optional primary image IDs, which is appropriate.Consider using camelCase for the field name to be consistent with Go naming conventions:
- PrimaryImageAssetId *AssetID `json:"primary_image_asset_id,omitempty"` + PrimaryImageAssetID *AssetID `json:"primary_image_asset_id,omitempty"`web/src/components/ui/file-upload.tsx (2)
83-86: Consider the necessity of re-exportingContextandHiddenInputIn lines 83-86, you are re-exporting
FileUploadContextasContextandFileUploadHiddenInputasHiddenInputfrom"@ark-ui/react/file-upload". Ensure that re-exporting these components aligns with your module's public API and that they are intended for external use. If these are internal components, it might be better to avoid exporting them to keep the API surface minimal.
15-81: Consider adding JSDoc comments to exported components for better documentationThe exported components
RootProvider,Root,Dropzone,ItemDeleteTrigger,ItemGroup,ItemName,ItemPreviewImage,ItemPreview,Item,ItemSizeText,Label, andTriggerare key parts of your UI library. Adding JSDoc comments to these components would improve code readability and provide better documentation for other developers using them.web/src/lib/library/library.ts (1)
170-172: Ensure deep clone when updatingprimary_imageWhen creating
newNode, you are using a shallow copy ofdatawith{ ...data, primary_image: asset }. Ifdatacontains nested objects, mutations to these nested objects elsewhere could affect this cached data unintentionally. Consider performing a deep clone to prevent unintended side effects.Apply the following change to use a deep cloning method:
- const newNode = { ...data, primary_image: asset }; + const newNode = { ...structuredClone(data), primary_image: asset };Alternatively, if the environment does not support
structuredClone, you can useJSON.parse(JSON.stringify(data))or a library likelodash.cloneDeep.web/src/screens/library/LibraryPageScreen/LibraryPageScreen.tsx (4)
104-107: Address the TODO: Implement custom icons or emojis for pagesThe TODO comment suggests adding custom icons or emojis for pages. Implementing this feature could enhance the user experience by allowing more personalization.
Would you like assistance in developing this feature or opening a GitHub issue to track its progress?
109-112: Address the TODO: Enable import from other sourcesThere's a TODO comment indicating the need to implement importing from other sources. Adding this functionality could improve the usability of the library by allowing users to bring in content from external platforms.
Would you like help in implementing this feature or creating a GitHub issue to track it?
123-123: Address the TODO: Remove black background when image is emptyThe TODO comment highlights the need to remove the black background when no image is present in the
FixedCropper. Implementing this would improve the visual appeal when the cover image is not set.Would you like assistance in resolving this issue or opening a GitHub issue to track it?
88-91: Simplify code by removing unnecessary fragmentThe fragment (
<></>) wrapping the<EditAction>component is unnecessary since there is only one child element. Removing it can simplify the code.Apply this diff to remove the unnecessary fragment:
<> <EditAction onClick={handleEditMode}>Edit</EditAction> - </>web/src/screens/library/LibraryPageScreen/useLibraryPageScreen.ts (1)
234-236: Address the TODO: Delete the original asset when updating the cover imageThe
TODOindicates that the original cover image asset should be deleted when a new one is uploaded. Implementing this will prevent accumulation of unused assets and ensure efficient storage management.Would you like assistance in implementing the deletion of the original asset, or should I open a new GitHub issue to track this task?
internal/ent/node_update.go (2)
166-185: Maintain Consistent Documentation and Ordering for New MethodsThe newly added methods for managing the
"primary_asset_id"field are correctly implemented. To maintain consistency with the rest of the codebase:
- Ensure that documentation comments follow the same style as existing methods.
- Consider placing these methods adjacent to other similar field methods for better organization.
1101-1120: Ensure Consistency inNodeUpdateOneMethod DocumentationThe methods for handling
"primary_asset_id"inNodeUpdateOneare implemented correctly. For consistency:
- Align the documentation comments with the existing style.
- Place the methods logically within the code to match the structure of similar methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
api/openapi.yamlis excluded by!**/*.yamlgo.sumis excluded by!**/*.sum,!**/*.sumweb/package.jsonis excluded by!**/*.jsonweb/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (35)
- app/resources/library/mapping.go (1 hunks)
- app/resources/library/node.go (1 hunks)
- app/resources/library/node_querier/node_querier.go (1 hunks)
- app/resources/library/node_search/search.go (1 hunks)
- app/resources/library/node_writer/node_writer.go (1 hunks)
- app/services/library/node_mutate/node.go (2 hunks)
- app/transports/http/bindings/nodes.go (4 hunks)
- app/transports/http/openapi/server_gen.go (6 hunks)
- internal/ent/client.go (1 hunks)
- internal/ent/er.html (2 hunks)
- internal/ent/migrate/schema.go (3 hunks)
- internal/ent/mutation.go (20 hunks)
- internal/ent/node.go (14 hunks)
- internal/ent/node/node.go (7 hunks)
- internal/ent/node/where.go (3 hunks)
- internal/ent/node_create.go (6 hunks)
- internal/ent/node_query.go (8 hunks)
- internal/ent/node_update.go (8 hunks)
- internal/ent/schema/node.go (2 hunks)
- web/panda.config.ts (2 hunks)
- web/src/api/openapi-schema/nodeCommonProps.ts (2 hunks)
- web/src/api/openapi-schema/nodeInitialProps.ts (2 hunks)
- web/src/api/openapi-schema/nodeMutableProps.ts (2 hunks)
- web/src/components/asset/AssetUploadAction.tsx (1 hunks)
- web/src/components/library/LibraryPageCoverImageControl/LibraryPageCoverImageControl.tsx (1 hunks)
- web/src/components/library/LibraryPageCoverImageControl/useLibraryPageCoverImageControl.ts (1 hunks)
- web/src/components/ui/file-upload.tsx (1 hunks)
- web/src/lib/library/library.ts (6 hunks)
- web/src/recipes/file-upload.ts (1 hunks)
- web/src/screens/library/LibraryPageScreen/LibraryPageScreen.tsx (3 hunks)
- web/src/screens/library/LibraryPageScreen/useLibraryPageScreen.ts (6 hunks)
- web/src/screens/media/MediaEditModal.tsx (1 hunks)
- web/src/screens/media/MediaEditScreen.tsx (1 hunks)
- web/styled-system/recipes/file-upload.d.ts (1 hunks)
- web/styled-system/recipes/file-upload.mjs (1 hunks)
🔥 Files not summarized due to errors (2)
- app/transports/http/openapi/server_gen.go: Error: Server error: no LLM provider could handle the message
- internal/ent/mutation.go: Error: Server error: no LLM provider could handle the message
🧰 Additional context used
🪛 Biome
web/src/components/asset/AssetUploadAction.tsx
[error] 66-66: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
🔇 Additional comments (89)
web/src/screens/media/MediaEditModal.tsx (1)
1-14: Overall assessment: Well-implemented modal component for media editing.This new
MediaEditModalcomponent aligns well with the PR objective of adding a primary asset cover image to library pages. It provides a clean interface for media editing within a modal context. The component is well-structured, uses appropriate props, and follows React best practices.A few minor optimizations have been suggested, but overall, this implementation looks good and should effectively serve its purpose in the larger context of the PR.
web/src/screens/media/MediaEditScreen.tsx (2)
7-10: Props type definition looks good.The
Propstype is well-defined with appropriate types forinitialValueandonFinish. The optionalinitialValueallows the component to be used for both creating new assets and editing existing ones.
16-28: Component structure looks good.The
MediaEditScreencomponent is well-structured. It correctly handles the upload process by calling theonFinishprop and renders theImageEditorcomponent with the necessary props. The use ofLStackfor layout is appropriate.web/src/components/library/LibraryPageCoverImageControl/useLibraryPageCoverImageControl.ts (6)
1-3: Imports look good and are well-organized.The necessary dependencies are imported, and there are no unused imports. The organization is logical, separating internal functions, types, and custom hooks.
5-7: Props type definition is clear and appropriate.The
Propstype is well-defined, exporting a singlenodeproperty of typeNode. This aligns with the PR objectives and provides a clear interface for the hook.
9-12: Hook setup and state management are well-implemented.The hook is correctly defined and uses
useLibraryMutationto obtain necessary functions. ThehasCoverImagestate is derived efficiently from the node'sprimary_imageproperty.
14-23: handleUploadCoverImage function is well-structured and handles errors appropriately.The function effectively manages the cover image upload process, using
handlefor error management and including a cleanup step to revalidate data. This implementation aligns well with the PR objectives.
25-34: handleRemoveCoverImage function is consistent with handleUploadCoverImage and effectively removes the cover image.The function maintains a consistent structure with
handleUploadCoverImage, properly handles errors, and includes a cleanup step. It correctly removes the cover image by passingundefinedtoupdateNodeCoverImage.
36-42: Return statement is well-structured and provides all necessary data and functions.The hook returns a clear object structure with
hasCoverImageand an object of handler functions. This return value aligns well with the PR objectives and allows for easy consumption of the hook's functionality.web/src/components/library/LibraryPageCoverImageControl/LibraryPageCoverImageControl.tsx (1)
1-9: LGTM: Imports are well-organized.The imports are properly structured, with external imports followed by internal imports. This organization enhances code readability and maintainability.
web/src/api/openapi-schema/nodeInitialProps.ts (3)
12-12: LGTM: Import statement for Identifier type.The new import statement for the
Identifiertype is correctly added and follows the existing import pattern in the file.
Line range hint
1-31: Verify changes in the source of truth for this auto-generated file.This file appears to be auto-generated, as indicated by the comment at the top. While the changes look good, it's crucial to ensure that these modifications are made in the source of truth (e.g., API schema, OpenAPI specification) that generates this file, rather than editing it directly.
Please confirm that the corresponding changes have been made in the source that generates this file. This ensures that the changes persist across future generations of this file.
27-27: LGTM: New property for primary image asset.The new
primary_image_asset_idproperty is correctly added to theNodeInitialPropsinterface. It's optional and uses theIdentifiertype, which aligns with the PR objectives.Let's verify the usage of this new property in the codebase:
✅ Verification successful
Verified:
primary_image_asset_idis correctly integrated across the codebase.
- The
primary_image_asset_idproperty is properly added and utilized in:
web/src/lib/library/library.tsweb/src/api/openapi-schema/nodeMutableProps.tsweb/src/api/openapi-schema/nodeInitialProps.tsweb/src/api/openapi-schema/eventMutableProps.tsweb/src/api/openapi-schema/eventInitialProps.tsweb/src/api/openapi-schema/nodeCreateBody.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new primary_image_asset_id property # Test: Search for usages of primary_image_asset_id rg --type typescript -A 5 'primary_image_asset_id' # Test: Check if there are any places where NodeInitialProps is used that might need updating rg --type typescript -A 5 'NodeInitialProps'Length of output: 165
Script:
#!/bin/bash # Description: Retry verifying the usage of the new primary_image_asset_id property with correct type flag # Test: Search for usages of primary_image_asset_id rg --type ts -A 5 'primary_image_asset_id' # Test: Check if there are any places where NodeInitialProps is used that might need updating rg --type ts -A 5 'NodeInitialProps'Length of output: 2553
web/src/api/openapi-schema/nodeMutableProps.ts (2)
12-12: LGTM: New import statement is correct and necessary.The new import statement for the
Identifiertype is correctly added and is necessary for the new property in theNodeMutablePropsinterface.
Line range hint
1-34: Verify changes in auto-generated file.This file appears to be auto-generated based on the comment at the top. While the changes are correct and necessary for the new feature, it's important to ensure that these modifications are made through the proper generation process rather than manual edits.
Please confirm that the changes to this file are the result of updates to the OpenAPI schema or the generation process, rather than direct manual edits.
web/styled-system/recipes/file-upload.d.ts (1)
19-19:⚠️ Potential issueRemove duplicate keys in the Record type
The return type of the function in the
FileUploadRecipeinterface contains repeated keys in theRecordtype. This repetition is unnecessary and may lead to confusion or potential issues in type checking.Consider simplifying the return type by removing the duplicate keys. Here's a suggested fix:
-(props?: FileUploadVariantProps): Pretty<Record<"root" | "dropzone" | "item" | "itemDeleteTrigger" | "itemGroup" | "itemName" | "itemPreview" | "itemPreviewImage" | "itemSizeText" | "label" | "trigger" | "root" | "dropzone" | "item" | "itemDeleteTrigger" | "itemGroup" | "itemName" | "itemPreview" | "itemPreviewImage" | "itemSizeText" | "label" | "trigger" | "root" | "dropzone" | "item" | "itemDeleteTrigger" | "itemGroup" | "itemName" | "itemPreview" | "itemPreviewImage" | "itemSizeText" | "label" | "trigger", string>> +(props?: FileUploadVariantProps): Pretty<Record<"root" | "dropzone" | "item" | "itemDeleteTrigger" | "itemGroup" | "itemName" | "itemPreview" | "itemPreviewImage" | "itemSizeText" | "label" | "trigger", string>>This change maintains the intended functionality while improving code clarity and reducing potential confusion.
To ensure this change doesn't affect other parts of the codebase, we can run the following script:
This will help us verify if there are any dependencies on the specific structure of the return type that might be affected by this change.
✅ Verification successful
Adjust the search command to target TypeScript files explicitly
The previous search using
rg --type typescriptfailed because thetypescriptfile type was not recognized. Let's search forFileUploadRecipewithin.tsand.tsxfiles instead.
Duplicate keys in Record type can be safely removed
The duplicate keys in the
Recordtype withinweb/styled-system/recipes/file-upload.d.tsare only present in this file and do not affect other parts of the codebase. Removing the duplicates will improve code clarity without introducing any issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of FileUploadRecipe interface # Test: Search for FileUploadRecipe usage rg --type typescript "FileUploadRecipe" -C 5Length of output: 79
Script:
#!/bin/bash # Description: Check for usage of FileUploadRecipe interface in TypeScript files # Test: Search for FileUploadRecipe usage in .ts and .tsx files rg "FileUploadRecipe" --glob "*.ts" --glob "*.tsx" -C 5Length of output: 1954
web/src/api/openapi-schema/nodeCommonProps.ts (1)
10-10: LGTM! The changes align with the PR objectives.The addition of the
primary_imageproperty to theNodeCommonPropsinterface correctly implements the feature for adding a primary asset cover image to library pages. The property is appropriately typed asAssetand marked as optional.However, please note that this file appears to be auto-generated. Ensure that any changes to this file are made through the appropriate generation process rather than direct edits.
Let's verify the impact of this change on the rest of the codebase:
Also applies to: 35-35
✅ Verification successful
Verified! The addition of
primary_imageis properly integrated across the codebase.All instances where
NodeCommonPropsis used have been appropriately updated to handle the newprimary_imageproperty. The optional nature of the property is respected, ensuring that existing functionality remains unaffected.No issues were found related to this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of NodeCommonProps and potential areas that might need updates # Search for files using NodeCommonProps echo "Files using NodeCommonProps:" rg --type typescript "NodeCommonProps" # Search for places where node properties are accessed, which might need to handle the new primary_image echo "\nPotential places to update for primary_image handling:" rg --type typescript "node\.(assets|content|description|link|meta|name|owner|parent|relevance_score|slug|visibility)"Length of output: 418
Script:
#!/bin/bash # Description: Corrected script to check for usage of NodeCommonProps and potential areas that might need updates # Search for files using NodeCommonProps echo "Files using NodeCommonProps:" rg "NodeCommonProps" --glob "*.ts" --glob "*.tsx" # Search for places where node properties are accessed, which might need to handle the new primary_image echo "\nPotential places to update for primary_image handling:" rg "node\.(assets|content|description|link|meta|name|owner|parent|relevance_score|slug|visibility)" --glob "*.ts" --glob "*.tsx"Length of output: 5905
Script:
#!/bin/bash # Description: Check for usage of the new 'primary_image' property across the codebase # Search for all occurrences of 'primary_image' echo "Files using 'primary_image':" rg "primary_image" --glob "*.ts" --glob "*.tsx"Length of output: 1534
app/resources/library/node_search/search.go (1)
73-73: LGTM! Consider verifying query performance.The addition of
WithPrimaryImage()aligns well with the PR objective of adding a primary image to library pages. This change will fetch the primary image data along with the node data, which is efficient.To ensure this addition doesn't negatively impact query performance, especially for large datasets, consider running the following verification:
If these tests reveal any issues or missing optimizations, consider addressing them before merging.
web/src/recipes/file-upload.ts (3)
1-2: LGTM: Imports are appropriateThe imports for
fileUploadAnatomyanddefineSlotRecipeare correctly specified and relevant to the file's purpose.
4-7: LGTM: Component definition is well-structuredThe
fileUploadcomponent is correctly defined usingdefineSlotRecipe. The use offileUploadAnatomy.keys()for slots ensures completeness, and the className "fileUpload" is appropriate.
8-74: LGTM: Comprehensive and well-structured stylesThe base styles for the file upload component are thorough and well-organized. The use of semantic color tokens, layout properties, and typography settings creates a cohesive and maintainable style structure. The grid layout for the item slot is particularly well-defined, and the addition of an animation for items is a nice touch for user experience.
app/resources/library/node.go (1)
42-42: LGTM! Verify handling in related methods.The addition of the
PrimaryImagefield to theNodestruct aligns well with the PR objective of adding a primary asset cover image to library pages. The use ofopt.Optional[asset.Asset]is appropriate, allowing the field to be optional.To ensure comprehensive implementation, please verify that related methods (e.g., creation, updating, and querying of nodes) handle this new field appropriately. Run the following script to check for potential places where updates might be needed:
✅ Verification successful
Further Verification Needed: Search for
PrimaryImageHandlingTo ensure that the new
PrimaryImagefield is properly handled across the codebase, please run the following scripts:
PrimaryImage Field Integration Verified
The new
PrimaryImagefield has been successfully integrated across the codebase. Key observations include:
- Proper implementation in data models (Event, Link, Node entities).
- CRUD operations (Create, Read, Update, Delete) for the field are in place.
- Query methods include the field for eager loading and filtering.
- API bindings correctly handle serialization and deserialization.
- Consistent implementation across different parts of the codebase.
The addition of the
PrimaryImagefield appears to be comprehensive and well-integrated, with no apparent issues or missing implementations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find potential places where the new PrimaryImage field might need to be handled # Search for Node struct usage in method signatures echo "Potential methods that might need updates:" rg --type go -e "func.*\(.*Node.*\)" -g '!app/resources/library/node.go' # Search for Node struct field access echo "\nPotential places accessing Node fields:" rg --type go -e "Node\.\w+" -g '!app/resources/library/node.go'Length of output: 175790
Script:
#!/bin/bash # Description: Find all usages and handling of the PrimaryImage field # Search for direct references to PrimaryImage echo "Searching for 'PrimaryImage' usages in the codebase:" rg --type go -e "PrimaryImage" # Search for methods that set or get PrimaryImage echo "\nSearching for methods related to 'PrimaryImage':" rg --type go -e "SetPrimaryImage|GetPrimaryImage|UpdatePrimaryImage|PrimaryImageOrErr" # Check for serialization/deserialization involving PrimaryImage echo "\nSearching for serialization/deserialization of 'PrimaryImage':" rg --type go -e "PrimaryImage\s*[:=]"Length of output: 44447
app/resources/library/mapping.go (1)
56-58: LGTM: Proper handling of optional PrimaryImageThe new code block correctly maps the optional
PrimaryImageedge to anasset.Asset. The use ofopt.Mapis appropriate for handling this optional field, and the conversion froment.Assettoasset.Assetusingasset.FromModelis consistent with the rest of the codebase.app/resources/library/node_querier/node_querier.go (1)
56-56: LGTM! Consider verifying query performance.The addition of
WithPrimaryImage()to the query chain aligns well with the PR objective of adding a primary asset cover image to library pages. This enhancement will allow theGetmethod to retrieve the primary image associated with the node.To ensure this change doesn't negatively impact query performance, especially for nodes without a primary image, consider running performance tests. You can use the following script to analyze the query execution time:
Don't forget to update any relevant documentation or comments to reflect this new functionality, if necessary.
app/resources/library/node_writer/node_writer.go (1)
76-80: LGTM: New WithPrimaryImage function implemented correctlyThe new
WithPrimaryImagefunction is well-implemented and follows the existing pattern of option functions in this file. It correctly sets the primary asset ID using theSetPrimaryAssetIDmethod of theent.NodeMutation.internal/ent/er.html (1)
165-165: LGTM: New field aligns with PR objectivesThe addition of the
primary_asset_idfield to theNodeentity is a good implementation of the PR's main objective. This field will allow library pages to have a designated primary image, which can serve as the cover image or hero section header.app/services/library/node_mutate/node.go (2)
47-47: LGTM: Addition of PrimaryImage fieldThe new
PrimaryImagefield of typeopt.Optional[asset.AssetID]is a good addition to thePartialstruct. It aligns with the PR objective of introducing a cover image for library pages and allows for optional updates.
65-65: LGTM: Update to Opts methodThe addition of
p.PrimaryImage.Call(func(value xid.ID) { opts = append(opts, node_writer.WithPrimaryImage(value)) })is correct and consistent with the handling of other fields in theOptsmethod. It ensures that thePrimaryImageoption is added only when a value is present.web/panda.config.ts (2)
14-14: New file upload functionality addedThe import of
fileUploadfrom@/recipes/file-uploadindicates the introduction of new file upload functionality. This aligns well with the PR objective of adding a primary asset cover image to library pages.
249-249: File upload recipe integrated into theme configurationThe
fileUploadrecipe has been added to theslotRecipessection of the theme configuration. This ensures that the new file upload component will have consistent styling with the rest of the application.To ensure the
fileUploadrecipe is properly implemented, let's verify its existence and structure:✅ Verification successful
File upload recipe is properly implemented and integrated into the theme configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the file-upload recipe exists and examine its content cat src/recipes/file-upload.tsLength of output: 91
Script:
#!/bin/bash # Search for files related to 'file-upload' in the repository fd 'file-upload' --type fLength of output: 179
Script:
#!/bin/bash # Search for occurrences of 'fileUpload' in the codebase rg 'fileUpload' .Length of output: 4571
Script:
#!/bin/bash # Display the contents of the correct file-upload recipe cat web/src/recipes/file-upload.tsLength of output: 1668
app/transports/http/bindings/nodes.go (2)
97-97: LGTM: PrimaryImage field added to node creationThe addition of the
PrimaryImagefield to thenode_mutate.Partialstruct is consistent and necessary for including the primary image in the node creation process.
Line range hint
1-424: Overall assessment: Primary image implementation looks goodThe changes in this file consistently implement the addition of a primary image field across node creation, update, and serialization functions. The modifications align well with the PR objectives and maintain the existing code style and patterns. Good job on maintaining consistency throughout the file.
To ensure the changes are comprehensive, let's verify if there are any other node-related functions that might need to include the
PrimaryImagefield:This will help us confirm that all relevant functions have been updated to handle the new
PrimaryImagefield.✅ Verification successful
To ensure that the
PrimaryImagefield is consistently handled across all node-related functions, let's verify its usage within each identified function.
Verification Completed:
PrimaryImageField is Consistently ImplementedThe
PrimaryImagefield is properly managed across all node-related functions inapp/transports/http/bindings/nodes.go. All relevant functions handlePrimaryImageappropriately, ensuring consistency and alignment with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other node-related functions that might need to include PrimaryImage rg --type go -i 'func.*node' app/transports/http/bindings/nodes.goLength of output: 1872
Script:
#!/bin/bash # Search for usages of PrimaryImage within node-related functions in nodes.go rg --type go 'PrimaryImage' app/transports/http/bindings/nodes.goLength of output: 436
internal/ent/node/where.go (3)
104-107: LGTM: New PrimaryAssetID function is correctly implemented.The new
PrimaryAssetIDfunction follows the established pattern for equality check predicates in this file. It's consistent with the existing code style and correctly implements the predicate for the "primary_asset_id" field.
674-752: LGTM: Comprehensive predicate functions added for primary_asset_id.The new predicate functions for
primary_asset_idare well-implemented and provide a comprehensive set of query operations. They follow the established patterns in the file and cover equality, inequality, range checks, containment, prefix/suffix matching, null checks, and case-insensitive comparisons. This addition enhances the querying capabilities for theNodeentity.
933-954: LGTM: Edge predicates for primary_image are correctly implemented.The new edge predicates
HasPrimaryImageandHasPrimaryImageWithfor the "primary_image" relationship are well-implemented. They follow the established patterns for edge predicates in this file, correctly setting up the M2O relationship and allowing for additional predicates on the relatedAssetentity. This addition enables efficient querying ofNodeentities based on their primary image relationship.internal/ent/client.go (1)
3539-3553: New methodQueryPrimaryImageadded toNodeClientThe addition of the
QueryPrimaryImagemethod to theNodeClientstruct is consistent with the PR objectives and follows the established pattern for edge query methods in this generated file. This new method will allow querying the primary image associated with a Node entity, which is crucial for implementing the cover image feature for library pages.internal/ent/mutation.go (16)
15629-15629: LGTM: Field count updated correctly.The increase in the field count from 12 to 13 correctly reflects the addition of the primary image field.
15657-15659: LGTM: Primary asset ID field correctly added.The addition of the primary asset ID field to the fields list is implemented correctly, following the existing pattern of checking for non-nil values before appending.
15695-15696: LGTM: Primary asset ID case added correctly.The new case for the primary asset ID field in the switch statement is implemented correctly, following the existing pattern of returning the corresponding method result.
15730-15731: LGTM: Old primary asset ID case added correctly.The new case for retrieving the old primary asset ID in the switch statement is implemented correctly, following the existing pattern of returning the corresponding method result with context.
15810-15816: LGTM: Primary asset ID setter case added correctly.The new case for setting the primary asset ID field is implemented correctly. It includes proper type assertion and error handling, following the existing pattern used for other fields.
15880-15882: LGTM: Primary asset ID field clearing check added correctly.The new condition to check if the primary asset ID field has been cleared is implemented correctly. It follows the existing pattern of using
m.FieldCleared()and appending the field to thefieldsslice if cleared.
15915-15917: LGTM: Primary asset ID field clearing case added correctly.The new case for clearing the primary asset ID field is implemented correctly. It follows the existing pattern of calling the corresponding clear method and returning nil.
15959-15961: LGTM: Primary asset ID field reset case added correctly.The new case for resetting the primary asset ID field is implemented correctly. It follows the existing pattern of calling the corresponding reset method and returning nil.
15977-15977: LGTM: Edges slice capacity updated correctly.The increase in the initial capacity of the
edgesslice from 8 to 9 correctly accounts for the addition of the new primary image edge.
15987-15989: LGTM: Primary image edge check added correctly.The new condition to check if the primary image has been set is implemented correctly. It follows the existing pattern of checking for non-nil values and appending the edge to the
edgesslice if set.
16026-16029: LGTM: Primary image edge case added correctly in EdgeIDs method.The new case for the primary image edge in the
EdgeIDsmethod is implemented correctly. It follows the existing pattern for singular edges, checking for a non-nil value and returning it as a slice ofent.Value.
16064-16064: LGTM: Removed edges slice capacity updated correctly.The increase in the initial capacity of the
edgesslice from 8 to 9 in theRemovedEdges()method correctly accounts for the addition of the new primary image edge.
16123-16123: LGTM: Cleared edges slice capacity updated correctly.The increase in the initial capacity of the
edgesslice from 8 to 9 in theClearedEdges()method correctly accounts for the addition of the new primary image edge.
16190-16192: LGTM: Primary image edge clearing case added correctly.The new case for clearing the primary image edge is implemented correctly. It follows the existing pattern of calling the corresponding clear method and returning nil.
16213-16215: LGTM: Primary image edge reset case added correctly.The new case for resetting the primary image edge is implemented correctly. It follows the existing pattern of calling the corresponding reset method and returning nil.
Line range hint
1-16215: Overall assessment: LGTM with minor suggestions.The changes to add the primary image field and edge to the Node entity are well-implemented and consistent with the existing patterns in the codebase. All necessary methods and cases have been added or updated correctly.
Key points:
- The primary image field and associated methods are correctly implemented.
- Edge-related methods are updated to include the new primary image edge.
- Some minor naming convention issues were identified, mainly related to using snake_case instead of camelCase for some variable and method names.
These changes align well with the PR objective of adding a primary asset cover image to library pages. The implementation appears to be thorough and should integrate smoothly with the existing codebase.
app/transports/http/openapi/server_gen.go (4)
1451-1452: LGTM: Addition of PrimaryImage fieldThe addition of the
PrimaryImagefield of type*Assetis consistent with the PR's objective of adding a primary asset cover image to library pages. The use of a pointer and theomitemptyJSON tag is appropriate for optional primary images.
1491-1492: Clarify the need for duplicate PrimaryImage fieldThis segment is identical to the previous addition of the
PrimaryImagefield. While this might be intentional for different structs or contexts, it would be helpful to clarify why this duplication is necessary. Could you provide more context on where these fields are being added and if there's a possibility for refactoring to avoid duplication?#!/bin/bash # Search for struct definitions containing the PrimaryImage field rg --type go -A 10 -B 5 'type \w+ struct \{(?:\n.*?)+PrimaryImage.*?\n'
1566-1568: Clarify the need for duplicate PrimaryImageAssetId fieldThis segment is identical to the previous addition of the
PrimaryImageAssetIdfield. As with the earlier duplication ofPrimaryImage, it would be helpful to understand why this duplication is necessary. Could you provide more context on where these fields are being added and if there's a possibility for refactoring to avoid duplication?#!/bin/bash # Search for struct definitions containing the PrimaryImageAssetId field rg --type go -A 10 -B 5 'type \w+ struct \{(?:\n.*?)+PrimaryImageAssetId.*?\n'
27732-27919: 🛠️ Refactor suggestionClarify the purpose of the large string block and consider alternatives
This large block of strings appears to be part of a
GetSwaggerfunction, possibly containing encoded or compressed swagger specification data. While this might be necessary, it raises some concerns:
- The purpose and necessity of this change are not immediately clear.
- Such a large block of inline strings can make the code harder to read and maintain.
Could you provide more context on why this change is needed?
Consider the following alternatives to improve code readability and maintainability:
- Store the swagger specification in a separate file and read it at runtime.
- If the data must be embedded, use a tool like go-bindata to generate Go code from the swagger file.
- If the current approach must be kept, add comments explaining the purpose and format of this data.
web/src/lib/library/library.ts (2)
236-236: IncludeupdateNodeCoverImagein returned mutationsAdding
updateNodeCoverImageto the returned object makes the function available for use elsewhere in the application, aligning with the intended functionality of updating a node's cover image.
177-179: Verify handling of undefinedprimary_image_asset_idin APIWhen
assetisundefined,primary_image_asset_idwill also beundefined. Ensure that thenodeUpdateAPI correctly interprets this to remove the cover image without causing errors. This is important for cases where a user wants to unset the cover image.Run the following script to confirm that passing
undefinedsuccessfully removes the cover image:Check the response to ensure that the
primary_imagefield is nownulland no errors are returned.internal/ent/node.go (7)
43-44: AddingPrimaryAssetIDfield toNodestructThe addition of the
PrimaryAssetID *xid.IDfield correctly introduces a reference to the primary asset within theNodestruct, allowing nodes to associate with a primary image asset as intended.
65-66: AddingPrimaryImageedge toNodeEdgesstructThe new
PrimaryImage *Assetfield in theNodeEdgesstruct properly defines the edge for the primary image relationship, enabling efficient querying and loading of the primary image associated with a node.
81-81: UpdatingloadedTypesarray sizeIncreasing the size of the
loadedTypesarray to[10]boolcorrectly accommodates the newPrimaryImageedge, ensuring proper tracking of loaded edge types during eager loading.
115-125: ImplementingPrimaryImageOrErrmethodThe
PrimaryImageOrErrmethod is correctly implemented to retrieve thePrimaryImageedge or return appropriate errors if it's not loaded or not found. The use ofe.loadedTypes[3]matches the index of thePrimaryImageedge, ensuring accurate error handling.
275-281: HandlingPrimaryAssetIDinassignValuesmethodThe
assignValuesmethod correctly handles the assignment of thePrimaryAssetIDfield. It checks for the appropriate type usingsql.NullScanner, verifies validity, and assigns the value properly, ensuring type safety and accurate data mapping from the database rows.
330-334: AddingQueryPrimaryImagemethodThe
QueryPrimaryImagemethod appropriately allows querying thePrimaryImageedge from aNodeentity. This addition enables retrieval of the primary image associated with a node, aligning with the PR objective of adding a cover image to library pages.
421-425: IncludingPrimaryAssetIDinStringmethod outputThe inclusion of
PrimaryAssetIDin theString()method ensures that the string representation of theNodeentity reflects the primary asset ID when it's notnil. This enhancement aids in debugging and logging by providing complete entity information.internal/ent/node/node.go (7)
37-38: Addition ofFieldPrimaryAssetIDThe
FieldPrimaryAssetIDconstant is correctly added to represent theprimary_asset_idfield in the database.
51-52: Addition ofEdgePrimaryImageThe
EdgePrimaryImageconstant is properly defined to denote theprimary_imageedge name in mutations.
82-88: Definition ofPrimaryImagerelation constantsThe constants
PrimaryImageTable,PrimaryImageInverseTable, andPrimaryImageColumnare correctly defined, establishing the relationship with theassetstable. This setup avoids circular dependencies with the "asset" package.
137-137: IncludingFieldPrimaryAssetIDinColumnsThe addition of
FieldPrimaryAssetIDto theColumnsslice ensures that the new field is included in SQL queries, which is essential for proper database operations involving this field.
262-266: AddingByPrimaryAssetIDOrderOptionThe
ByPrimaryAssetIDfunction is correctly implemented, allowing results to be ordered by theprimary_asset_idfield. This enhances query capabilities for sorting based on the new field.
305-311: AddingByPrimaryImageFieldOrderOptionThe
ByPrimaryImageFieldfunction is properly added, enabling ordering by a specified field of theprimary_imageedge. This provides flexibility in sorting queries based on related asset attributes.
409-415: DefiningnewPrimaryImageStepThe
newPrimaryImageStepfunction is correctly defined to establish the SQL graph step for theprimary_imagerelationship in queries. This step is crucial for traversing the relationship between nodes and their primary images.internal/ent/migrate/schema.go (3)
543-543: Addition of 'primary_asset_id' column is correctThe new column
primary_asset_idis properly defined as a nullable string with a size of 20. This aligns with the existing schema conventions and allows nodes to reference a primary asset.
569-574: Foreign key constraint for 'primary_asset_id' is appropriately setThe foreign key
nodes_assets_primary_imagecorrectly establishes a relationship between thenodestable and theassetstable. It referencesAssetsColumns[0](which corresponds to theidcolumn inassets) fromNodesColumns[13](the newly addedprimary_asset_idcolumn). TheOnDelete: schema.SetNullaction ensures that if a referenced asset is deleted, theprimary_asset_idinnodeswill be set toNULL, which is appropriate for maintaining data integrity.
1014-1014: Initialization of foreign key reference is correctly implementedIn the
initfunction, the assignment toNodesTable.ForeignKeys[3].RefTable = AssetsTablecorrectly sets the reference table for the new foreign key. This ensures that the schema migration will properly recognize the relationship betweennodesandassets.internal/ent/node_query.go (7)
36-36: LGTM!The addition of
withPrimaryImagefield is consistent with existing patterns.
495-495: LGTM!The
Clonemethod correctly includes cloning ofwithPrimaryImage.
542-552: LGTM!The
WithPrimaryImagemethod is implemented consistently with existing eager-loading methods.
Line range hint
697-707: LGTM!The
loadedTypesarray is correctly updated with the new edgewithPrimaryImage.
750-755: LGTM!The code correctly loads the
PrimaryImageedge when eager-loading is requested.
888-919: LGTM!The
loadPrimaryImagemethod correctly handles the loading of thePrimaryImageedge, including null checks forPrimaryAssetID.
1258-1260: LGTM!The code correctly ensures that
PrimaryAssetIDis selected whenwithPrimaryImageis requested.internal/ent/node_create.go (5)
134-138: Method addition looks goodThe
SetPrimaryAssetIDmethod is correctly implemented and follows the standard pattern for setting fields in theNodeCreatebuilder.
140-146: Nillable method implementation is consistentThe
SetNillablePrimaryAssetIDmethod is properly implemented, allowing for nullable assignment ofPrimaryAssetID.
807-823:NodeUpsertmethods forPrimaryAssetIDare correctly implementedThe methods
SetPrimaryAssetID,UpdatePrimaryAssetID, andClearPrimaryAssetIDin theNodeUpsertstruct are properly implemented, allowing for setting and updating thePrimaryAssetIDfield during upsert operations.
1064-1084:NodeUpsertOnemethods forPrimaryAssetIDare properly addedThe addition of
SetPrimaryAssetID,UpdatePrimaryAssetID, andClearPrimaryAssetIDmethods inNodeUpsertOneis correctly implemented, enabling individual upsert operations to manage thePrimaryAssetIDfield.
1499-1519:NodeUpsertBulkmethods forPrimaryAssetIDare correctly implementedThe methods
SetPrimaryAssetID,UpdatePrimaryAssetID, andClearPrimaryAssetIDin theNodeUpsertBulkstruct are properly implemented, facilitating bulk upsert operations for thePrimaryAssetIDfield.internal/ent/node_update.go (2)
399-404: Verify the Necessity of Clearing the "primary_image" EdgeThe
ClearPrimaryImage()method allows the"primary_image"edge to be unset. Please confirm:
- This behavior aligns with application requirements.
- The system can handle nodes without a primary image without issues.
If nodes must always have a primary image, consider removing this method or adding checks to prevent unintended clearing.
1334-1339: Confirm Clearing Behavior of "primary_image" Edge inNodeUpdateOneThe
ClearPrimaryImage()method allows the removal of the"primary_image"edge. Please verify:
- This operation is intended and handled appropriately in the application logic.
- There are no adverse effects when a node lacks a primary image.
| export function useMediaEditScreen(props: Props) { | ||
| // | ||
| } |
There was a problem hiding this comment.
Implement or remove empty hook.
The useMediaEditScreen hook is currently empty. If it's intended for future use, consider adding a TODO comment explaining its purpose. If it's not needed, remove it to avoid confusion.
If you decide to keep it for future implementation, you could add a TODO comment like this:
export function useMediaEditScreen(props: Props) {
// TODO: Implement media edit screen logic
}Alternatively, if it's not needed, you can remove it entirely:
-export function useMediaEditScreen(props: Props) {
- //
-}| } | ||
|
|
||
| export function MediaEditScreen(props: Props) { | ||
| useMediaEditScreen(props); |
There was a problem hiding this comment.
Reconsider the use of useMediaEditScreen hook.
The useMediaEditScreen hook is called, but its return value is not used. This could lead to unnecessary computations or side effects. Consider removing the hook call if it's not needed, or use its return value if it provides essential functionality.
If the hook is not needed, you can remove it like this:
-useMediaEditScreen(props);If it is needed, make sure to use its return value:
const hookResult = useMediaEditScreen(props);
// Use hookResult in your component| @@ -24,6 +24,7 @@ func (Node) Fields() []ent.Field { | |||
| field.String("content").Optional().Nillable(), | |||
| field.String("parent_node_id").GoType(xid.ID{}).Optional(), | |||
| field.String("account_id").GoType(xid.ID{}), | |||
| field.String("primary_asset_id").GoType(xid.ID{}).Optional().Nillable(), | |||
There was a problem hiding this comment.
💡 Codebase verification
Potential issues with handling primary_asset_id during Asset deletion.
The search did not find any OnDelete operations handling the primary_asset_id references in Nodes. This could lead to dangling references if an Asset is deleted without updating or removing the corresponding primary_asset_id in Nodes. It's recommended to implement cascade delete or nullification strategies to maintain data consistency.
🔗 Analysis chain
Consider implications on querying and data consistency.
The addition of the primary_asset_id field and primary_image edge enhances the Node schema to support a cover image, aligning well with the PR objectives. However, consider the following points:
-
Querying: Ensure that queries fetching Node data are updated to include the primary image when necessary, to avoid N+1 query problems.
-
Data Consistency: Implement checks to ensure that the
primary_asset_idalways corresponds to an existing Asset, perhaps using ent's hooks or validators. -
Cascade Behavior: Consider what should happen to this reference if the associated Asset is deleted. You might want to add a policy to handle this scenario.
-
Migration: Ensure there's a migration plan for existing Node entries that don't have a primary image set.
To verify the usage and potential impact, you can run the following script:
Also applies to: 54-57
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing Node queries and potential places that need updating
# Search for Node queries that might need updating
echo "Searching for Node queries that might need updating:"
rg --type go 'Node\s*\{[^}]*\}' -g '!internal/ent/schema/*'
# Search for potential places where data consistency checks might be needed
echo "\nSearching for potential places where data consistency checks might be needed:"
rg --type go 'Create.*Node' -g '!internal/ent/schema/*'
# Search for existing delete operations on Asset that might affect Nodes
echo "\nSearching for existing delete operations on Asset that might affect Nodes:"
rg --type go 'Delete.*Asset' -g '!internal/ent/schema/*'
Length of output: 46088
Script:
#!/bin/bash
# Description: Verify inclusion of 'primary_asset_id' in Node queries
# Check if 'primary_asset_id' is being selected in Node queries
echo "Checking if 'primary_asset_id' is included in Node queries:"
rg --type go 'primary_asset_id' -g '!internal/ent/schema/*' 'Node'
# Description: Verify validators or hooks for 'primary_asset_id' in Node creation and updates
echo "\nChecking for validators or hooks enforcing 'primary_asset_id':"
rg --type go 'SetPrimaryAssetID' -g '!internal/ent/schema/*'
# Description: Ensure Asset deletion operations handle 'primary_asset_id' in Nodes
echo "\nChecking if Asset deletion handles 'primary_asset_id' references in Nodes:"
rg --type go 'OnDelete' -g '!internal/ent/schema/*' 'primary_asset_id'
Length of output: 4305
Script:
#!/bin/bash
# Description: Verify inclusion of 'primary_asset_id' in Node queries
# Corrected search for 'primary_asset_id' within Node-related queries
echo "Checking if 'primary_asset_id' is included in Node queries:"
rg --type go 'Node\s*\{[^}]*primary_asset_id[^}]*\}' -g '!internal/ent/schema/*'
# Description: Verify validators or hooks for 'primary_asset_id' in Node creation and updates
echo "\nChecking for validators or hooks enforcing 'primary_asset_id':"
rg --type go 'SetPrimaryAssetID' -g '!internal/ent/schema/*'
# Description: Ensure Asset deletion operations handle 'primary_asset_id' references in Nodes
echo "\nChecking if Asset deletion handles 'primary_asset_id' references in Nodes:"
rg --type go 'OnDelete.*primary_asset_id' -g '!internal/ent/schema/*'
Length of output: 4224
| [ | ||
| "trigger", | ||
| "fileUpload__trigger" | ||
| ], | ||
| [ | ||
| "root", | ||
| "fileUpload__root" | ||
| ], | ||
| [ | ||
| "dropzone", | ||
| "fileUpload__dropzone" | ||
| ], | ||
| [ | ||
| "item", | ||
| "fileUpload__item" | ||
| ], | ||
| [ | ||
| "itemDeleteTrigger", | ||
| "fileUpload__itemDeleteTrigger" | ||
| ], | ||
| [ | ||
| "itemGroup", | ||
| "fileUpload__itemGroup" | ||
| ], | ||
| [ | ||
| "itemName", | ||
| "fileUpload__itemName" | ||
| ], | ||
| [ | ||
| "itemPreview", | ||
| "fileUpload__itemPreview" | ||
| ], | ||
| [ | ||
| "itemPreviewImage", | ||
| "fileUpload__itemPreviewImage" | ||
| ], | ||
| [ | ||
| "itemSizeText", | ||
| "fileUpload__itemSizeText" | ||
| ], | ||
| [ | ||
| "label", | ||
| "fileUpload__label" | ||
| ], |
There was a problem hiding this comment.
Remove duplicate slot definitions
The added slot definitions from lines 92 to 135 are exact duplicates of existing entries in the fileUploadSlotNames array. This duplication is unnecessary and could lead to confusion, maintenance issues, and potential performance impacts.
Please remove these duplicate entries. The fileUploadSlotNames array should contain only one instance of each slot definition. Apply this diff to resolve the issue:
- [
- "trigger",
- "fileUpload__trigger"
- ],
- [
- "root",
- "fileUpload__root"
- ],
- [
- "dropzone",
- "fileUpload__dropzone"
- ],
- [
- "item",
- "fileUpload__item"
- ],
- [
- "itemDeleteTrigger",
- "fileUpload__itemDeleteTrigger"
- ],
- [
- "itemGroup",
- "fileUpload__itemGroup"
- ],
- [
- "itemName",
- "fileUpload__itemName"
- ],
- [
- "itemPreview",
- "fileUpload__itemPreview"
- ],
- [
- "itemPreviewImage",
- "fileUpload__itemPreviewImage"
- ],
- [
- "itemSizeText",
- "fileUpload__itemSizeText"
- ],
- [
- "label",
- "fileUpload__label"
- ],📝 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.
| [ | |
| "trigger", | |
| "fileUpload__trigger" | |
| ], | |
| [ | |
| "root", | |
| "fileUpload__root" | |
| ], | |
| [ | |
| "dropzone", | |
| "fileUpload__dropzone" | |
| ], | |
| [ | |
| "item", | |
| "fileUpload__item" | |
| ], | |
| [ | |
| "itemDeleteTrigger", | |
| "fileUpload__itemDeleteTrigger" | |
| ], | |
| [ | |
| "itemGroup", | |
| "fileUpload__itemGroup" | |
| ], | |
| [ | |
| "itemName", | |
| "fileUpload__itemName" | |
| ], | |
| [ | |
| "itemPreview", | |
| "fileUpload__itemPreview" | |
| ], | |
| [ | |
| "itemPreviewImage", | |
| "fileUpload__itemPreviewImage" | |
| ], | |
| [ | |
| "itemSizeText", | |
| "fileUpload__itemSizeText" | |
| ], | |
| [ | |
| "label", | |
| "fileUpload__label" | |
| ], |
| func WithPrimaryImage(id asset.AssetID) Option { | ||
| return func(nm *ent.NodeMutation) { | ||
| nm.SetPrimaryAssetID(id) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a function to remove the primary image
While the current implementation allows setting a primary image, it might be useful to provide a way to remove it as well. Consider adding a WithoutPrimaryImage function that clears the primary asset ID.
Here's a suggested implementation:
func WithoutPrimaryImage() Option {
return func(nm *ent.NodeMutation) {
nm.ClearPrimaryAsset()
}
}This assumes that the ent.NodeMutation has a ClearPrimaryAsset method. If it doesn't, you might need to implement it or use a different approach to clear the primary asset.
| func (nc *NodeCreate) SetNillablePrimaryImageID(id *xid.ID) *NodeCreate { | ||
| if id != nil { | ||
| nc = nc.SetPrimaryImageID(*id) | ||
| } | ||
| return nc | ||
| } |
There was a problem hiding this comment.
Remove unnecessary reassignment in SetNillablePrimaryImageID
In the SetNillablePrimaryImageID method, reassigning nc with nc = nc.SetPrimaryImageID(*id) is unnecessary since SetPrimaryImageID modifies nc and returns it. This reassignment could be omitted for consistency and clarity.
Apply this fix to remove the unnecessary reassignment:
func (nc *NodeCreate) SetNillablePrimaryImageID(id *xid.ID) *NodeCreate {
if id != nil {
- nc = nc.SetPrimaryImageID(*id)
+ nc.SetPrimaryImageID(*id)
}
return nc
}📝 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.
| func (nc *NodeCreate) SetNillablePrimaryImageID(id *xid.ID) *NodeCreate { | |
| if id != nil { | |
| nc = nc.SetPrimaryImageID(*id) | |
| } | |
| return nc | |
| } | |
| func (nc *NodeCreate) SetNillablePrimaryImageID(id *xid.ID) *NodeCreate { | |
| if id != nil { | |
| nc.SetPrimaryImageID(*id) | |
| } | |
| return nc | |
| } |
| // SetPrimaryImageID sets the "primary_image" edge to the Asset entity by ID. | ||
| func (nu *NodeUpdate) SetPrimaryImageID(id xid.ID) *NodeUpdate { | ||
| nu.mutation.SetPrimaryImageID(id) | ||
| return nu | ||
| } | ||
|
|
||
| // SetNillablePrimaryImageID sets the "primary_image" edge to the Asset entity by ID if the given value is not nil. | ||
| func (nu *NodeUpdate) SetNillablePrimaryImageID(id *xid.ID) *NodeUpdate { | ||
| if id != nil { | ||
| nu = nu.SetPrimaryImageID(*id) | ||
| } | ||
| return nu | ||
| } | ||
|
|
||
| // SetPrimaryImage sets the "primary_image" edge to the Asset entity. | ||
| func (nu *NodeUpdate) SetPrimaryImage(a *Asset) *NodeUpdate { | ||
| return nu.SetPrimaryImageID(a.ID) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add Validation for the "primary_image" Edge
While the methods for setting the "primary_image" edge are correctly implemented, it's advisable to add validation to ensure:
- The
Assetbeing assigned exists and is valid. - The
Assetmeets any criteria required to be a primary image (e.g., correct file type or status).
This can prevent runtime errors and maintain data integrity.
| // SetPrimaryImageID sets the "primary_image" edge to the Asset entity by ID. | ||
| func (nuo *NodeUpdateOne) SetPrimaryImageID(id xid.ID) *NodeUpdateOne { | ||
| nuo.mutation.SetPrimaryImageID(id) | ||
| return nuo | ||
| } | ||
|
|
||
| // SetNillablePrimaryImageID sets the "primary_image" edge to the Asset entity by ID if the given value is not nil. | ||
| func (nuo *NodeUpdateOne) SetNillablePrimaryImageID(id *xid.ID) *NodeUpdateOne { | ||
| if id != nil { | ||
| nuo = nuo.SetPrimaryImageID(*id) | ||
| } | ||
| return nuo | ||
| } | ||
|
|
||
| // SetPrimaryImage sets the "primary_image" edge to the Asset entity. | ||
| func (nuo *NodeUpdateOne) SetPrimaryImage(a *Asset) *NodeUpdateOne { | ||
| return nuo.SetPrimaryImageID(a.ID) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Validation for "primary_image" Methods in NodeUpdateOne
Similar to the NodeUpdate struct, it's advisable to:
- Add validation to ensure the
Assetassigned as the primary image is valid and exists. - Prevent possible runtime errors due to invalid references.
| if nuo.mutation.PrimaryImageCleared() { | ||
| edge := &sqlgraph.EdgeSpec{ | ||
| Rel: sqlgraph.M2O, | ||
| Inverse: false, | ||
| Table: node.PrimaryImageTable, | ||
| Columns: []string{node.PrimaryImageColumn}, | ||
| Bidi: false, | ||
| Target: &sqlgraph.EdgeTarget{ | ||
| IDSpec: sqlgraph.NewFieldSpec(asset.FieldID, field.TypeString), | ||
| }, | ||
| } | ||
| _spec.Edges.Clear = append(_spec.Edges.Clear, edge) | ||
| } | ||
| if nodes := nuo.mutation.PrimaryImageIDs(); len(nodes) > 0 { | ||
| edge := &sqlgraph.EdgeSpec{ | ||
| Rel: sqlgraph.M2O, | ||
| Inverse: false, | ||
| Table: node.PrimaryImageTable, | ||
| Columns: []string{node.PrimaryImageColumn}, | ||
| Bidi: false, | ||
| Target: &sqlgraph.EdgeTarget{ | ||
| IDSpec: sqlgraph.NewFieldSpec(asset.FieldID, field.TypeString), | ||
| }, | ||
| } | ||
| for _, k := range nodes { | ||
| edge.Target.Nodes = append(edge.Target.Nodes, k) | ||
| } | ||
| _spec.Edges.Add = append(_spec.Edges.Add, edge) | ||
| } |
There was a problem hiding this comment.
Improve Error Handling in sqlSave for NodeUpdateOne
In the sqlSave method for NodeUpdateOne, consider enhancing error handling when updating the "primary_image" edge:
- Check for the existence of the
AssetID provided. - Handle scenarios where the edge assignment could fail due to referential integrity constraints.
An example modification:
if nodes := nuo.mutation.PrimaryImageIDs(); len(nodes) > 0 {
edge := &sqlgraph.EdgeSpec{
Rel: sqlgraph.M2O,
Inverse: false,
Table: node.PrimaryImageTable,
Columns: []string{node.PrimaryImageColumn},
Bidi: false,
Target: &sqlgraph.EdgeTarget{
IDSpec: sqlgraph.NewFieldSpec(asset.FieldID, field.TypeString),
},
}
for _, k := range nodes {
edge.Target.Nodes = append(edge.Target.Nodes, k)
+ // Verify Asset existence
+ exists, err := AssetExists(ctx, nuo.driver, k)
+ if err != nil {
+ return nil, err
+ }
+ if !exists {
+ return nil, fmt.Errorf("ent: Asset with ID %v does not exist", k)
+ }
}
_spec.Edges.Add = append(_spec.Edges.Add, edge)
}Committable suggestion was skipped due to low confidence.
| if nu.mutation.PrimaryImageCleared() { | ||
| edge := &sqlgraph.EdgeSpec{ | ||
| Rel: sqlgraph.M2O, | ||
| Inverse: false, | ||
| Table: node.PrimaryImageTable, | ||
| Columns: []string{node.PrimaryImageColumn}, | ||
| Bidi: false, | ||
| Target: &sqlgraph.EdgeTarget{ | ||
| IDSpec: sqlgraph.NewFieldSpec(asset.FieldID, field.TypeString), | ||
| }, | ||
| } | ||
| _spec.Edges.Clear = append(_spec.Edges.Clear, edge) | ||
| } | ||
| if nodes := nu.mutation.PrimaryImageIDs(); len(nodes) > 0 { | ||
| edge := &sqlgraph.EdgeSpec{ | ||
| Rel: sqlgraph.M2O, | ||
| Inverse: false, | ||
| Table: node.PrimaryImageTable, | ||
| Columns: []string{node.PrimaryImageColumn}, | ||
| Bidi: false, | ||
| Target: &sqlgraph.EdgeTarget{ | ||
| IDSpec: sqlgraph.NewFieldSpec(asset.FieldID, field.TypeString), | ||
| }, | ||
| } | ||
| for _, k := range nodes { | ||
| edge.Target.Nodes = append(edge.Target.Nodes, k) | ||
| } | ||
| _spec.Edges.Add = append(_spec.Edges.Add, edge) | ||
| } |
There was a problem hiding this comment.
Enhance Error Handling for "primary_image" Edge Updates in sqlSave
In the sqlSave method, you've added logic to handle the "primary_image" edge. To improve robustness:
- Add error handling for cases where the provided
AssetID does not correspond to an existing entity. - Ensure that setting or clearing the edge maintains database integrity.
Consider updating the code as follows:
if nodes := nu.mutation.PrimaryImageIDs(); len(nodes) > 0 {
edge := &sqlgraph.EdgeSpec{
Rel: sqlgraph.M2O,
Inverse: false,
Table: node.PrimaryImageTable,
Columns: []string{node.PrimaryImageColumn},
Bidi: false,
Target: &sqlgraph.EdgeTarget{
IDSpec: sqlgraph.NewFieldSpec(asset.FieldID, field.TypeString),
},
}
for _, k := range nodes {
edge.Target.Nodes = append(edge.Target.Nodes, k)
}
_spec.Edges.Add = append(_spec.Edges.Add, edge)
+ // Add error handling for non-existent Asset IDs
+ exists, err := AssetExists(ctx, nu.driver, k)
+ if err != nil {
+ return 0, err
+ }
+ if !exists {
+ return 0, fmt.Errorf("ent: Asset with ID %v does not exist", k)
+ }
}Committable suggestion was skipped due to low confidence.
This also introduces the concept of assets having version history. The need for this was because when a cover image is cropped, the original is still needed in order to facilitate future edits to the crop.
Southclaws
force-pushed
the
page-primary-images
branch
from
db2fcdb to
ed2a469
Compare
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range and nitpick comments (28)
web/src/api/openapi-schema/asset.ts (1)
17-17: LGTM! Consider implementation details for recursive structure.The addition of the optional
parentproperty of typeAssetto theAssetinterface is a good approach to implement the recursive backreference mentioned in the PR objectives. This change enables a hierarchical organization of assets, which can be beneficial for managing relationships between assets, such as the original and cropped versions of images mentioned in the PR summary.However, when implementing this recursive structure, consider the following:
- Ensure that circular references are prevented or handled appropriately to avoid infinite loops during serialization or traversal.
- Implement proper depth limiting when querying or displaying asset hierarchies to prevent performance issues with deeply nested structures.
- Update any existing code that works with
Assetobjects to handle this new optional property correctly.web/src/api/openapi-schema/parentAssetIDQueryParameter.ts (1)
11-19: LGTM: Well-documented type definitionThe
ParentAssetIDQueryParametertype is clearly defined, and the accompanying documentation provides comprehensive information about its purpose and usage scenarios. This aligns well with the PR objectives of introducing a new asset relation for library pages.Consider adding a brief example of how this parameter might be used in a request, to further clarify its application for developers.
internal/ent/schema/asset.go (3)
31-31: LGTM! Consider adding a comment for clarity.The addition of the
parent_asset_idfield is well-implemented. It's correctly defined as optional and nillable, allowing for assets without a parent. The use ofxid.ID{}as the Go type is consistent with other ID fields in the schema.Consider adding a brief comment above the field to explain its purpose:
+ // ID of the parent asset, if this asset is a derivative or version of another asset field.String("parent_asset_id").GoType(xid.ID{}).Optional().Nillable(),
58-62: LGTM! Consider adding inverse edges for completeness.The new edge is well-implemented, establishing a clear parent-child relationship between assets. The "Unique" constraint correctly ensures that an asset can have only one parent.
For improved querying capabilities, consider adding an inverse edge from parent to children:
edge.To("assets", Asset.Type). From("parent"). Unique(). - Field("parent_asset_id"), + Field("parent_asset_id"). + Inverse("children"),This would allow easy querying of an asset's children, enhancing the API's flexibility.
31-31: Consider implications of asset hierarchiesThe introduction of parent-child relationships for assets is a significant change that may have broader implications for asset management and querying.
Recursive Querying: Consider implementing methods or hooks that allow for efficient recursive querying of asset hierarchies. This could be useful for scenarios where you need to traverse the entire asset tree.
Depth Limitation: Evaluate whether there should be a limit on the depth of asset hierarchies to prevent potential performance issues with deeply nested structures.
Circular References: Implement safeguards to prevent circular references in the asset hierarchy (e.g., asset A being a parent of asset B, which is a parent of asset A).
Cascade Operations: Consider how operations like deletion should behave in the context of asset hierarchies. Should deleting a parent asset also delete all its children?
These considerations align with the PR objective of discussing recursive backreferencing for Assets. Implementing these features now or planning for them in the future could enhance the robustness of the asset management system.
Also applies to: 58-62
web/src/api/openapi-schema/assetUploadParams.ts (2)
34-42: LGTM: New property added with clear documentationThe new
parent_asset_idproperty is well-defined and documented. It aligns with the PR objectives and maintains backward compatibility by being optional. The comment provides clear guidance on its usage.Consider adding a brief example in the comment to illustrate how this property might be used in practice. This could help developers quickly understand its application. For instance:
/** * ...existing comment... * * Example: * { * filename: "updated_image.jpg", * parent_asset_id: "asset_123" * } */
Line range hint
1-42: Overall assessment: Well-implemented feature additionThe changes in this file successfully introduce the new asset relation feature for library pages. The implementation is backward-compatible, well-documented, and follows existing code patterns. These modifications align well with the PR objectives, particularly in supporting the handling of original and cropped versions of images.
As the project evolves, consider creating a separate type for version-related properties if more properties are added in the future. This could improve code organization and make it easier to manage version-specific logic.
app/resources/asset/db.go (1)
42-61: Good implementation, but consider adding validation and documentationThe
AddVersionmethod is well-structured and consistent with the existingAddmethod. It correctly sets the parent asset ID, which aligns with the PR objective of adding versioning to assets. The error handling is robust, using thefaultpackage for wrapping errors with context and tags.However, there are a few areas for improvement:
- Consider adding validation to ensure the parent asset exists before creating a new version.
- Implement permission checking to ensure the account has the right to add a version to the parent asset.
- Add a comment to explain the purpose of the
AddVersionmethod and how it differs fromAdd.Here's a suggested improvement:
+// AddVersion creates a new version of an existing asset. +// It links the new asset to its parent using the parent's AssetID. func (d *database) AddVersion(ctx context.Context, accountID xid.ID, parent AssetID, filename Filename, size int, ) (*Asset, error) { + // Verify that the parent asset exists + _, err := d.GetByID(ctx, parent) + if err != nil { + return nil, fault.Wrap(err, fctx.With(ctx), ftag.With(ftag.InvalidArgument)) + } + + // TODO: Implement permission checking to ensure the account can add a version to this asset + asset, err := d.db.Asset. Create(). SetID(filename.GetID()). SetParentAssetID(parent). SetFilename(filename.name). SetSize(size). SetAccountID(xid.ID(accountID)). Save(ctx) if err != nil { return nil, fault.Wrap(err, fctx.With(ctx), ftag.With(ftag.Internal)) } return FromModel(asset), nil }This improvement adds a check for the parent asset's existence and a placeholder for permission checking. It also includes a comment explaining the method's purpose.
web/src/components/ui/file-upload.tsx (2)
15-22: New RootProvider component with strong typingThe introduction of the
RootProvidercomponent with its complex type definition enhances type safety and provides a clear contract for component props. This is a positive addition to the file.For improved readability, consider using type aliases for the complex type definitions. For example:
type RootProviderPropsType = Assign< Assign<HTMLStyledProps<"div">, FileUpload.RootProviderBaseProps>, FileUploadVariantProps >; export const RootProvider = withProvider<HTMLDivElement, RootProviderPropsType>( FileUpload.RootProvider, "root" );This approach can make the code more maintainable and easier to understand at a glance.
24-31: Consistent typing approach for Root componentThe update to the
Rootcomponent aligns it with the typing approach used inRootProvider. This consistency is commendable and improves the overall structure of the file.As suggested for
RootProvider, consider using a type alias here as well:type RootPropsType = Assign< Assign<HTMLStyledProps<"div">, FileUpload.RootBaseProps>, FileUploadVariantProps >; export const Root = withProvider<HTMLDivElement, RootPropsType>( FileUpload.Root, "root" );This will further enhance readability and maintainability.
app/transports/http/bindings/assets.go (1)
51-52: LGTM! Consider adding a comment for clarity.The addition of
parentIDaligns well with the PR objective of introducing a new asset relation. The use ofopt.NewPtrMapis a good practice for handling optional values.Consider adding a brief comment explaining the purpose of this new variable:
+// Extract the optional parent asset ID from the request parameters parentID := opt.NewPtrMap(request.Params.ParentAssetId, deserialiseAssetID)web/src/components/asset/AssetUploadAction.tsx (1)
96-147: Rendering logic looks good, minor improvement for getMIMEs function.The component's rendering logic is clear and follows React best practices. The
getMIMEsfunction handles different input types correctly. However, there's room for a small improvement:In the
getMIMEsfunction, consider using type narrowing to simplify the logic:function getMIMEs( accept: Record<string, string[]> | string | string[] | undefined ): string[] { if (!accept) return []; if (typeof accept === "string") return [accept]; if (Array.isArray(accept)) return accept; return Object.keys(accept); }This change makes the function more concise and easier to read.
The rendering logic and overall structure of the component are well-implemented.
web/src/screens/library/LibraryPageScreen/LibraryPageScreen.tsx (2)
30-38: Improved data fetching and error handlingThe use of
useNodeGethook with fallback data and the implementation of error handling withUnreadyBannerare excellent improvements. They enhance data management and user experience.Consider adding a loading state to provide feedback to users while data is being fetched:
if (isLoading) { return <LoadingSpinner />; } if (!data) { return <UnreadyBanner error={error} />; }
81-169: Improved UI for editing and cover image managementThe simplification of edit actions and the implementation of cover image cropping are well-executed and align with the PR objectives. The conditional rendering effectively handles different states of the component.
Consider extracting the
FixedCropperlogic into a separate component to improve readability and maintainability:const CoverImageCropper = ({ url, initialCoordinates, onCrop }) => { // ... cropper logic }; // Usage {editing && primaryAssetEditingURL && ( <CoverImageCropper url={primaryAssetEditingURL} initialCoordinates={initialCoverCoordinates} onCrop={handleCoverAdjustment} /> )}internal/ent/er.html (2)
244-244: LGTM: New relationships addedThe new relationships
Node }o--o| Asset : "primary_image"andAsset |o--o{ Asset : "assets/parent"correctly represent the primary image feature and the parent-child relationship between assets. These additions are consistent with the PR objectives and the new fields added to the entities.Consider renaming the
"assets/parent"relationship to"children/parent"for clarity. This would make it more intuitive that an asset can have multiple children, while still being a child of another asset.Also applies to: 264-264
43-43: Summary: ERD updated to support cover images and asset relationshipsThe changes to the ERD successfully implement the structure needed for the cover image feature and introduce a more flexible asset relationship model. These modifications include:
- Adding a parent-child relationship for assets
- Introducing a primary asset for nodes (potentially all node types)
- Establishing the necessary relationships between nodes, assets, and other assets
These changes not only fulfill the PR objectives but also provide a foundation for potential future enhancements to asset management within the system. The implementation maintains consistency with the existing ERD structure while introducing new capabilities.
Consider documenting these new relationships and their potential uses in the project's architecture documentation. This will help other developers understand the full capabilities of the new asset structure and how it can be leveraged in different parts of the application.
Also applies to: 166-166, 244-244, 264-264
app/services/library/node_mutate/node.go (1)
48-48: LGTM: Addition of PrimaryImage field.The new
PrimaryImagefield of typedeletable.Value[asset.AssetID]in thePartialstruct aligns with the PR objective of adding a primary asset cover image to library pages. This allows for setting, unsetting, or removing the primary image for a node.Consider adding a comment to explain the purpose and behavior of the
PrimaryImagefield, especially regarding itsdeletable.Valuenature.web/src/lib/library/library.ts (1)
40-61: LGTM: Well-definedCoverImageArgstype with clear documentation.The
CoverImageArgstype is well-structured and provides clear options for handling cover images with or without cropping. The comments are helpful in explaining the purpose of each field.Consider adding a brief comment above the type definition to explain its overall purpose and usage context.
internal/ent/migrate/schema.go (2)
123-123: LGTM! Consider adding an index forparent_asset_id.The addition of the
parent_asset_idcolumn and its corresponding foreign key constraint is well-implemented. This change allows for a hierarchical structure of assets, which aligns with the PR objectives. TheSetNullaction on delete is a good choice for maintaining data integrity.Consider adding an index on the
parent_asset_idcolumn to improve query performance when fetching child assets:{Name: "parent_asset_id", Type: field.TypeString, Nullable: true, Size: 20}, +// Add this line +{Name: "parent_asset_id_index", Unique: false, Columns: []*schema.Column{AssetsColumns[7]}},Also applies to: 137-142
550-550: LGTM! Consider adding an index forprimary_asset_id.The addition of the
primary_asset_idcolumn and its corresponding foreign key constraint is well-implemented. This change allows nodes to have a primary asset (cover image), which directly aligns with the PR objectives. TheSetNullaction on delete is a good choice for maintaining data integrity.Consider adding an index on the
primary_asset_idcolumn to improve query performance when fetching nodes with their primary assets:{Name: "primary_asset_id", Type: field.TypeString, Nullable: true, Size: 20}, +// Add this line +{Name: "primary_asset_id_index", Unique: false, Columns: []*schema.Column{NodesColumns[13]}},Also applies to: 576-581
internal/ent/node_create.go (1)
241-258: New methods for PrimaryImage edge look good, with a minor suggestionThe new methods
SetPrimaryImageID,SetNillablePrimaryImageID, andSetPrimaryImageare correctly implemented and follow the existing patterns in the codebase. They provide a way to set the "primary_image" edge to the Asset entity, which aligns with the PR objective.However, there's a small inconsistency in the
SetNillablePrimaryImageIDmethod:In the
SetNillablePrimaryImageIDmethod, consider removing the unnecessary assignment:func (nc *NodeCreate) SetNillablePrimaryImageID(id *xid.ID) *NodeCreate { if id != nil { - nc = nc.SetPrimaryImageID(*id) + nc.SetPrimaryImageID(*id) } return nc }This change would make it consistent with other similar methods in the file.
internal/ent/mutation.go (4)
4131-4178: LGTM: Comprehensive methods for parent asset ID management.The new methods for handling the parent asset ID are well-implemented and provide a complete set of operations. They follow the established pattern in the codebase.
One minor suggestion:
Consider adding a comment to explain the purpose of the
OldParentAssetIDmethod, as it's not immediately obvious why it's needed.+// OldParentAssetID returns the old value of the parent_asset_id field before any changes in this mutation. +// This is used for optimistic concurrency control in update operations. func (m *AssetMutation) OldParentAssetID(ctx context.Context) (v *xid.ID, err error) { // ... (existing implementation) }
4382-4474: LGTM: Comprehensive methods for parent-child asset relationship management.The new methods for handling the parent asset relationship and assets relationship are well-implemented and provide a complete set of operations. They follow the established pattern in the codebase.
One minor suggestion:
Consider adding a brief comment to explain the purpose of the
ParentIDsmethod, as it's not immediately clear why it returns a slice when there's only one parent.+// ParentIDs returns the parent IDs in the mutation. +// Note: This method is used internally by the framework and should not be used directly. func (m *AssetMutation) ParentIDs() (ids []xid.ID) { // ... (existing implementation) }
15210-15257: LGTM: Comprehensive methods for primary asset ID management.The new methods for handling the primary asset ID are well-implemented and provide a complete set of operations. They follow the established pattern in the codebase and include proper error handling.
One minor suggestion:
Consider adding a comment to explain the purpose of the
OldPrimaryAssetIDmethod, as it's not immediately obvious why it's needed.+// OldPrimaryAssetID returns the old value of the primary_asset_id field before any changes in this mutation. +// This is used for optimistic concurrency control in update operations. func (m *NodeMutation) OldPrimaryAssetID(ctx context.Context) (v *xid.ID, err error) { // ... (existing implementation) }
15527-15565: LGTM: Comprehensive methods for primary image relationship management.The new methods for handling the primary image relationship are well-implemented and provide a complete set of operations. They follow the established pattern in the codebase.
One minor suggestion:
Consider adding a brief comment to explain the purpose of the
PrimaryImageIDsmethod, as it's not immediately clear why it returns a slice when there's only one primary image.+// PrimaryImageIDs returns the primary image IDs in the mutation. +// Note: This method is used internally by the framework and should not be used directly. func (m *NodeMutation) PrimaryImageIDs() (ids []xid.ID) { // ... (existing implementation) }internal/deletable/deletable.go (2)
8-12: Clarify the documentation forValue[T]structThe current documentation may cause confusion because both the "deleted" and "left untouched" states are associated with "no value." Consider rephrasing to differentiate these states more clearly. For example:
- Deleted: No value,
delete = true- Updated: Holds a new value
- Unchanged: Neither deleted nor updated (no value)
13-16: Consider renaming thedeletefield to avoid confusion with built-in functionsUsing
deleteas a field name might lead to confusion sincedeleteis a built-in function in Go for deleting map entries. Consider renaming the field toshouldDeleteormarkForDeletionfor better clarity.app/services/asset/asset_upload/uploader.go (1)
51-53: Add documentation comments for exported struct fieldsThe exported struct
Optionshas fieldsContentFilland the newly addedParentIDwithout documentation comments. According to Go conventions, exported fields should have comments for better readability and maintainability.Consider adding comments like:
type Options struct { // ContentFill specifies how the content should be filled. ContentFill opt.Optional[asset.ContentFillCommand] // ParentID is the optional ID of the parent asset for versioning. ParentID opt.Optional[asset.AssetID] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (6)
api/config.yamlis excluded by!**/*.yamlapi/openapi.yamlis excluded by!**/*.yamlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumweb/package.jsonis excluded by!**/*.jsonweb/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (53)
- app/resources/asset/asset.go (3 hunks)
- app/resources/asset/db.go (1 hunks)
- app/resources/library/mapping.go (1 hunks)
- app/resources/library/node.go (1 hunks)
- app/resources/library/node_querier/node_querier.go (1 hunks)
- app/resources/library/node_search/search.go (1 hunks)
- app/resources/library/node_writer/node_writer.go (1 hunks)
- app/services/asset/asset_upload/uploader.go (2 hunks)
- app/services/library/node_mutate/node.go (3 hunks)
- app/transports/http/bindings/assets.go (2 hunks)
- app/transports/http/bindings/nodes.go (5 hunks)
- app/transports/http/openapi/server_gen.go (13 hunks)
- internal/deletable/deletable.go (1 hunks)
- internal/ent/asset.go (7 hunks)
- internal/ent/asset/asset.go (7 hunks)
- internal/ent/asset/where.go (3 hunks)
- internal/ent/asset_create.go (6 hunks)
- internal/ent/asset_query.go (8 hunks)
- internal/ent/asset_update.go (8 hunks)
- internal/ent/client.go (2 hunks)
- internal/ent/er.html (4 hunks)
- internal/ent/migrate/schema.go (6 hunks)
- internal/ent/mutation.go (41 hunks)
- internal/ent/node.go (14 hunks)
- internal/ent/node/node.go (7 hunks)
- internal/ent/node/where.go (3 hunks)
- internal/ent/node_create.go (6 hunks)
- internal/ent/node_query.go (8 hunks)
- internal/ent/node_update.go (8 hunks)
- internal/ent/schema/asset.go (2 hunks)
- internal/ent/schema/node.go (2 hunks)
- web/panda.config.ts (2 hunks)
- web/src/api/openapi-schema/asset.ts (1 hunks)
- web/src/api/openapi-schema/assetUploadParams.ts (2 hunks)
- web/src/api/openapi-schema/index.ts (1 hunks)
- web/src/api/openapi-schema/nodeCommonProps.ts (2 hunks)
- web/src/api/openapi-schema/nodeInitialProps.ts (2 hunks)
- web/src/api/openapi-schema/nodeMutableProps.ts (2 hunks)
- web/src/api/openapi-schema/nullableIdentifier.ts (1 hunks)
- web/src/api/openapi-schema/parentAssetIDQueryParameter.ts (1 hunks)
- web/src/components/asset/AssetUploadAction.tsx (1 hunks)
- web/src/components/library/LibraryPageCoverImageControl/LibraryPageCoverImageControl.tsx (1 hunks)
- web/src/components/library/LibraryPageCoverImageControl/useLibraryPageCoverImageControl.ts (1 hunks)
- web/src/components/ui/file-upload.tsx (1 hunks)
- web/src/lib/library/library.ts (7 hunks)
- web/src/lib/library/metadata.ts (1 hunks)
- web/src/recipes/file-upload.ts (1 hunks)
- web/src/screens/library/LibraryPageScreen/LibraryPageScreen.tsx (3 hunks)
- web/src/screens/library/LibraryPageScreen/useLibraryPageScreen.ts (7 hunks)
- web/src/screens/media/MediaEditModal.tsx (1 hunks)
- web/src/screens/media/MediaEditScreen.tsx (1 hunks)
- web/styled-system/recipes/file-upload.d.ts (1 hunks)
- web/styled-system/recipes/file-upload.mjs (1 hunks)
🔥 Files not summarized due to errors (2)
- app/transports/http/openapi/server_gen.go: Error: Server error: no LLM provider could handle the message
- internal/ent/mutation.go: Error: Server error: no LLM provider could handle the message
🚧 Files skipped from review as they are similar to previous changes (17)
- app/resources/library/mapping.go
- app/resources/library/node.go
- app/resources/library/node_querier/node_querier.go
- app/resources/library/node_search/search.go
- app/transports/http/bindings/nodes.go
- internal/ent/schema/node.go
- web/panda.config.ts
- web/src/api/openapi-schema/nodeCommonProps.ts
- web/src/api/openapi-schema/nodeInitialProps.ts
- web/src/api/openapi-schema/nodeMutableProps.ts
- web/src/components/library/LibraryPageCoverImageControl/LibraryPageCoverImageControl.tsx
- web/src/components/library/LibraryPageCoverImageControl/useLibraryPageCoverImageControl.ts
- web/src/recipes/file-upload.ts
- web/src/screens/media/MediaEditModal.tsx
- web/src/screens/media/MediaEditScreen.tsx
- web/styled-system/recipes/file-upload.d.ts
- web/styled-system/recipes/file-upload.mjs
🧰 Additional context used
🪛 Biome
web/src/components/asset/AssetUploadAction.tsx
[error] 68-68: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
🔇 Additional comments (167)
web/src/lib/library/metadata.ts (2)
1-3: LGTM: Imports are correct and necessary.The imports for Zod and the Metadata type are appropriate for the functionality of this file.
11-14: LGTM: NodeMetadataSchema and type are well-defined.The NodeMetadataSchema is correctly structured with an optional coverImage property. This aligns well with the PR objective of adding a cover image to library pages while maintaining flexibility for nodes without cover images.
web/src/api/openapi-schema/nullableIdentifier.ts (2)
1-9: LGTM: Well-structured generated file headerThe generated comment header provides crucial information about the file's origin and the API it pertains to. It clearly indicates that the file shouldn't be manually edited and provides valuable insights into the API's versioning strategy. This information is essential for developers working with the Storyden API.
11-15: LGTM: Clear and well-documented type definitionThe
NullableIdentifiertype is well-defined and properly documented. The use of a union type (string | null) accurately represents a nullable string identifier. The JSDoc comment clearly explains the type's purpose and includes the@nullabletag, which is consistent with the type definition.web/src/api/openapi-schema/asset.ts (1)
Line range hint
1-24: Verify API schema consistency for auto-generated file.This file appears to be auto-generated based on the OpenAPI schema. To ensure the integrity of the API contract:
- Verify that the changes in this file accurately reflect the intended modifications to the API schema.
- Confirm that the auto-generation process is working correctly and that no manual changes were made to this file.
- If there are any discrepancies, update the source OpenAPI schema and regenerate the TypeScript definitions.
To check for any manual modifications to this auto-generated file, you can run the following command:
If this command returns any output, it indicates that there might be uncommitted changes to the file, which should be investigated as it's supposed to be auto-generated.
✅ Verification successful
No Uncommitted Changes Detected in
asset.ts.The auto-generated file
web/src/api/openapi-schema/asset.tshas no uncommitted changes, confirming its consistency with the OpenAPI schema and ensuring the auto-generation process is functioning correctly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any uncommitted changes in the auto-generated file git status --porcelain web/src/api/openapi-schema/asset.tsLength of output: 60
web/src/api/openapi-schema/parentAssetIDQueryParameter.ts (1)
1-9: LGTM: File header provides crucial contextThe file header effectively communicates that this is an auto-generated file and provides important information about the Storyden API's versioning strategy. This context is valuable for developers working with the API.
web/src/api/openapi-schema/assetUploadParams.ts (1)
13-13: LGTM: New import added correctlyThe new import for
ParentAssetIDQueryParameteris correctly added and follows the existing import pattern in the file. This import is necessary for the new property added to theAssetUploadParamstype.web/src/components/ui/file-upload.tsx (4)
1-13: Improved modularity and type safety in importsThe new import structure and the introduction of
createStyleContextdemonstrate a move towards a more modular and type-safe approach. This change enhances maintainability and provides better IntelliSense support.
33-81: Consistent and type-safe approach for FileUpload componentsThe redefinition of multiple FileUpload components (Dropzone, ItemDeleteTrigger, ItemGroup, etc.) using
withContextand specific HTML element types is a significant improvement. This approach:
- Enhances type safety by explicitly defining the HTML element type for each component.
- Improves consistency across the component definitions.
- Facilitates easier styling and context sharing among components.
The consistent structure makes the code more maintainable and easier to understand. Great job on implementing this standardized approach across all components!
83-86: Improved module exports with clear namingThe addition of exports for
FileUploadContextasContextandFileUploadHiddenInputasHiddenInputenhances the flexibility of this module. The clear naming convention improves the overall usability and readability of the code when these components are imported in other parts of the application.
58-61:⚠️ Potential issueFix inconsistent HTML element type for ItemPreview
The
ItemPreviewcomponent still has an inconsistency between its HTML element type and props type. The first type parameter isHTMLImageElement, but the props useHTMLStyledProps<"div">. This mismatch could lead to incorrect usage.To resolve this, you should align the element type with the props type. If
ItemPreviewis intended to be adivelement, update the type as follows:export const ItemPreview = withContext< HTMLDivElement, Assign<HTMLStyledProps<"div">, FileUpload.ItemPreviewBaseProps> >(FileUpload.ItemPreview, "itemPreview");Alternatively, if it's meant to be an
imgelement, update the props type accordingly.app/transports/http/bindings/assets.go (1)
59-59: LGTM! Consistent with the new parent asset feature.The addition of the
ParentIDfield to theasset_upload.Optionsstruct is consistent with the new parent asset feature. This change allows the upload function to associate an asset with its parent, supporting the new asset relation mentioned in the PR objectives.web/src/components/asset/AssetUploadAction.tsx (3)
1-22: Imports and type definitions look good!The imports are appropriate for the component's functionality, and the type definitions for
AssetUploadActionPropsandPropsare clear and well-structured.
24-29: Component setup looks good, but verify getMIMEs function.The component setup follows React best practices and demonstrates good separation of concerns. However, ensure that the
getMIMEsfunction (called on line 29) is properly implemented and tested.#!/bin/bash # Description: Verify the implementation of the getMIMEs function # Expected: The function should be defined and handle different input types correctly # Test: Search for the getMIMEs function definition rg --type typescript 'function getMIMEs' # Test: Check if the function handles different input types (string, array, record) rg --type typescript 'if \(typeof accept === "string"\)' -A 10
31-47: Improve error handling and investigate potential bug.The
handleFilefunction seems to handle file uploads correctly, but consider the following improvements:
- Enhance error handling by providing more specific error messages, especially for the case when no file is provided.
- Investigate the potential Zag bug mentioned in the comment on line 33. If it's a known issue, consider adding a link to the issue tracker or providing more context.
Consider updating the error logging on line 36 to provide more context:
- console.error("handleFile: no file was provided", files); + console.error("handleFile: expected at least one file, but none were provided", { files });app/resources/library/node_writer/node_writer.go (3)
76-80: LGTM: WithPrimaryImage function implementationThe
WithPrimaryImagefunction is well-implemented and aligns with the PR objectives. It correctly uses theOptionpattern and sets the primary asset ID as expected.
82-86: LGTM: WithPrimaryImageRemoved function implementationThe
WithPrimaryImageRemovedfunction is well-implemented and addresses the suggestion from the past review comment. It correctly uses theOptionpattern and clears the primary asset ID as expected.
76-86: Excellent addition of primary image management functionsThe new
WithPrimaryImageandWithPrimaryImageRemovedfunctions are well-implemented and significantly enhance the node management capabilities. These additions align perfectly with the PR objectives of adding a primary asset cover image to library pages. They also address the past review comment suggestion, demonstrating responsiveness to feedback.These functions will allow for more flexible and robust handling of primary images in the library pages, improving the overall user experience.
web/src/screens/library/LibraryPageScreen/LibraryPageScreen.tsx (3)
3-4: Improved component structure and necessary imports addedThe changes to the import statements and the introduction of the
LibraryPagecomponent are well-structured and align with the PR objectives. The separation of concerns betweenLibraryPageScreenandLibraryPageenhances code organization and maintainability.Also applies to: 11-11, 13-13, 17-17, 20-23, 30-38
Line range hint
40-58: Well-structured component props and state managementThe addition of new props related to the cover image functionality and the use of the
useLibraryPageScreencustom hook demonstrate good practices in React component design. This approach effectively separates concerns and manages complex state logic outside the component.
154-166:⚠️ Potential issueFix prop types for Next.js Image component
The use of the Next.js Image component for displaying the cover image is appropriate. However, the
widthandheightprops should be numbers, not strings.Please update the Image component props as follows:
<Image className={css({ width: "full", height: "full", borderRadius: "lg", objectFit: "cover", objectPosition: "center", })} src={primaryAssetURL} alt="" - width="1920" - height="1080" + width={1920} + height={1080} />internal/ent/er.html (2)
43-43: LGTM: New parent-child relationship for assetsThe addition of
parent_asset_idto theAssetentity enables a hierarchical structure for assets. This change aligns with the PR objectives and allows for more complex asset relationships, which could be useful for managing different versions or variations of an asset.
166-166: LGTM: Primary asset for nodes addedThe addition of
primary_asset_idto theNodeentity allows nodes to have a primary asset, which aligns with the PR objective of adding a cover image to library pages. This implementation suggests that the feature is available for all nodes, not just library pages.Could you clarify if this feature is intended to be available for all node types or specifically for library pages? If it's for all nodes, consider updating the PR description to reflect this broader scope.
app/services/library/node_mutate/node.go (3)
29-29: LGTM: New import for deletable package.The addition of the
deletablepackage import is consistent with the newPrimaryImagefield in thePartialstruct. This import is necessary for using thedeletable.Valuetype.
66-70: LGTM: Proper handling of PrimaryImage in Opts method.The new logic for handling the
PrimaryImagefield in theOptsmethod is well-implemented. It correctly uses theCallmethod ofdeletable.Valueto handle all possible states (set, unset, removed) of the primary image. The implementation is consistent with how other fields are processed in this method.
Line range hint
102-270: Verify: Implicit handling of PrimaryImage in Create and Update methods.The
CreateandUpdatemethods don't show explicit changes for handling the newPrimaryImagefield. This suggests that the processing ofPrimaryImageis abstracted away in theOptsmethod or thenode_writerpackage, which is a good practice for maintaining separation of concerns.To ensure proper handling of the
PrimaryImagefield, please verify:
- The
node_writerpackage correctly processes theWithPrimaryImageandWithPrimaryImageRemovedoptions.- The
CreateandUpdatemethods in thenode_writerpackage handle these new options appropriately.You can use the following script to check the
node_writerpackage:web/src/lib/library/library.ts (7)
3-3: LGTM: New imports and types for cover image functionality.The added imports and types are appropriate for implementing the cover image feature. They provide the necessary tools and type definitions for handling assets and node metadata.
Also applies to: 9-9, 17-19, 33-34
288-288: LGTM: AddedremoveNodeCoverImageto the returned objectThe addition of
removeNodeCoverImageto the returned object fromuseLibraryMutationis consistent with the implementation of the new function. This makes the new functionality available to consumers of this hook.
Line range hint
1-314: Overall assessment: Good implementation with room for improvementThe changes to
library.tssuccessfully implement the cover image functionality, introducing new types, functions, and modifying existing ones to support this feature. The code is generally well-structured and follows good practices.Main points for improvement:
- Enhance error handling, particularly in functions like
removeNodeCoverImageandupdateNode.- Improve type safety in areas like the
mergePrimaryImageAssetfunction and theupdateNodefunction's use of type assertions.- Ensure consistency in the structure and behavior of mutation functions.
- Address the potential issue with query parameter handling in
nodeListAllKeyFn.- Clarify the behavior for edge cases, such as when
oldNode.primary_imageis undefined inmergePrimaryImageAsset.These improvements will enhance the robustness and maintainability of the code. Consider implementing the suggested changes and adding comprehensive unit tests to verify the behavior of the new and modified functions.
Line range hint
135-190: Improvements needed inupdateNodefunctionThe
updateNodefunction has been updated to handle cover image updates, which is great. However, there are a few areas that need attention:
The
nodeMutatorfunction uses the spread operator to merge properties, which could lead to unexpected results ifnewNodecontains properties not present in the original data.The TODO comment on line 187 indicates that the original node metadata should be spread, but it's not being done.
The type assertion
satisfies NodeWithChildrenon line 151 might hide type errors if the merged object doesn't actually satisfy theNodeWithChildrentype.To address these issues, consider the following improvements:
- Use a more explicit merging strategy for
nodeProps:const nodeProps = Object.fromEntries( Object.entries(newNode).filter(([key]) => key in data && key !== 'parent') );
- Implement the spreading of the original node metadata:
const newMeta = cover && !cover.isReplacement ? { ...data.meta, coverImage: cover.config, } : undefined;
- Replace the type assertion with a type guard:
const updated: Partial<NodeWithChildren> = { ...data, ...nodeProps, ...withNewCover, }; if (isNodeWithChildren(updated)) { return updated; } else { console.error('Updated node does not satisfy NodeWithChildren type'); return data; }Where
isNodeWithChildrenis a type guard function you'd need to implement.To verify these changes, we can add some unit tests:
#!/bin/bash # Create a test file for updateNode function cat << EOF > test_updateNode.ts import { updateNode } from './library'; import { NodeWithChildren, CoverImageArgs } from './types'; // Mock data const mockNode: NodeWithChildren = { // ... (fill with mock data) }; const mockCover: CoverImageArgs = { // ... (fill with mock data) }; // Test cases console.log(updateNode('test-slug', { name: 'New Name' })); console.log(updateNode('test-slug', { name: 'New Name' }, mockCover)); console.log(updateNode('test-slug', { invalidProp: 'Should not be included' } as any)); EOF # Run the test file npx ts-node test_updateNode.tsThis test script will help verify that the
updateNodefunction correctly handles various scenarios, including invalid properties and cover image updates.
211-232: 🛠️ Refactor suggestionImprove error handling and consistency in
removeNodeCoverImageThe
removeNodeCoverImagefunction correctly implements the removal of cover images with optimistic updates. However, there are a few areas for improvement:
- Error handling is missing for the
nodeUpdatecall.- The function's structure differs slightly from other mutation functions in the file.
Consider the following improvements:
- Add error handling:
try { await nodeUpdate(slug, { primary_image_asset_id: null, }); } catch (error) { console.error("Failed to remove node cover image:", error); // Revert the optimistic update await mutate(nodeKeyFn); }
- For consistency with other functions, consider moving the
mutatecall after thenodeUpdate:const mutator: MutatorCallback<NodeGetOKResponse> = (data) => { if (!data) return; return { ...data, primary_image: undefined }; }; try { await nodeUpdate(slug, { primary_image_asset_id: null, }); await mutate(nodeKeyFn, mutator, { revalidate: false }); } catch (error) { console.error("Failed to remove node cover image:", error); // Optionally, show an error message to the user }These changes will improve error handling and make the function more consistent with the rest of the codebase.
To verify these changes, we can add a unit test:
#!/bin/bash # Create a test file for removeNodeCoverImage function cat << EOF > test_removeNodeCoverImage.ts import { removeNodeCoverImage } from './library'; import { NodeWithChildren } from './types'; // Mock SWR's mutate function const mockMutate = jest.fn(); jest.mock('swr', () => ({ ...jest.requireActual('swr'), useSWRConfig: () => ({ mutate: mockMutate }), })); // Mock nodeUpdate function const mockNodeUpdate = jest.fn(); jest.mock('@/api/openapi-client/nodes', () => ({ ...jest.requireActual('@/api/openapi-client/nodes'), nodeUpdate: mockNodeUpdate, })); describe('removeNodeCoverImage', () => { it('should remove cover image and update node', async () => { const slug = 'test-slug'; await removeNodeCoverImage(slug); expect(mockMutate).toHaveBeenCalled(); expect(mockNodeUpdate).toHaveBeenCalledWith(slug, { primary_image_asset_id: null }); }); it('should handle errors and revert optimistic update', async () => { const slug = 'test-slug'; mockNodeUpdate.mockRejectedValueOnce(new Error('Update failed')); await removeNodeCoverImage(slug); expect(mockMutate).toHaveBeenCalledTimes(2); // Once for optimistic update, once for revert expect(console.error).toHaveBeenCalled(); }); }); EOF # Run the test file npx jest test_removeNodeCoverImage.tsThis test script will help verify that the
removeNodeCoverImagefunction correctly handles both successful and error scenarios.
72-78:⚠️ Potential issueAddress potential issue with query parameters in
nodeListAllKeyFnThe updated function improves readability by using template literals. However, the comment on line 76 suggests a potential issue when handling keys with query parameters.
To address this, consider implementing a more robust key matching logic. Here's a possible solution:
const nodeListAllKeyFn = (key: Arguments) => { if (!Array.isArray(key) || typeof key[0] !== 'string') return false; const [baseUrl] = key[0].split('?'); return baseUrl.startsWith(nodeListKey) && !baseUrl.startsWith(`${nodeListKey}/`); };This approach splits the key by the query parameter separator and only checks the base URL part, which should handle cases with query parameters correctly.
To verify this solution, we can run the following test:
This test script will help verify that the proposed solution correctly handles various scenarios, including those with query parameters.
295-314: 🛠️ Refactor suggestionImprove type safety and clarity in
mergePrimaryImageAssetThe
mergePrimaryImageAssetfunction is a good addition for handling optimistic updates of the primary image asset. However, there are a few areas where it could be improved:
- The function's return type could be more specific.
- The logic for determining the
parentAssetcould be clearer.- There's no handling for the case where
oldNode.primary_imageis undefined.Consider the following improvements:
- Specify a more precise return type:
function mergePrimaryImageAsset( oldNode: Node, coverConfig: CoverImageArgs ): Pick<Node, "primary_image" | "meta"> { // ... function body ... }
- Clarify the
parentAssetlogic:const parentAsset = oldNode.primary_image?.parent ?? oldNode.primary_image ?? null;
- Handle the case where
oldNode.primary_imageis undefined:if (coverConfig.isReplacement || !oldNode.primary_image) { return { primary_image: coverConfig.asset, meta: { ...oldNode.meta, coverImage: coverConfig.isReplacement ? null : coverConfig.config }, }; }Could you clarify the intended behavior when
oldNode.primary_imageis undefined? Should it be treated the same as a replacement, or should it be handled differently?To verify these changes, we can add some unit tests:
This test script will help verify that the
mergePrimaryImageAssetfunction correctly handles various scenarios, including replacements, non-replacements, and cases where the original node has no primary image.internal/ent/node.go (7)
43-44: LGTM: New field for primary asset addedThe addition of the
PrimaryAssetIDfield as a pointer toxid.IDis consistent with other ID fields in theNodestruct and allows for optional primary asset association.
65-66: LGTM: New edge for primary image addedThe addition of the
PrimaryImageedge in theNodeEdgesstruct and the corresponding increase in theloadedTypesarray size are appropriate changes to support the new primary asset feature.Also applies to: 81-81
187-188: LGTM: Scan value handling for primary asset ID addedThe addition of the case for
node.FieldPrimaryAssetIDin thescanValuesmethod is implemented correctly. It usessql.NullScannerwith a newxid.ID, which is consistent with how other ID fields are handled in this method.
275-281: LGTM: Assignment handling for primary asset ID addedThe addition of the case for
node.FieldPrimaryAssetIDin theassignValuesmethod is implemented correctly. It properly checks for the correct type, handles the nullable nature of the field, and is consistent with how other nullable ID fields are handled in this method.
330-333: LGTM: New query method for primary image addedThe
QueryPrimaryImagemethod is implemented correctly, following the same pattern as other query methods in theNodestruct. It properly returns anAssetQueryobject, allowing for querying the primary image associated with a node.
Line range hint
1-437: Summary: Primary asset feature successfully implementedThe changes in this file successfully implement the primary asset feature for the Node entity. The additions include:
- A new
PrimaryAssetIDfield in theNodestruct- A new
PrimaryImageedge in theNodeEdgesstruct- A new
PrimaryImageOrErrmethod for error handling- Updates to
scanValuesandassignValuesmethods to handle the new field- A new
QueryPrimaryImagemethod for querying the primary imageThese changes are consistent with the existing code structure and patterns, and they effectively support the PR objective of adding a primary asset cover image to library pages.
115-124: LGTM: New method for primary image edge addedThe
PrimaryImageOrErrmethod is implemented correctly, following the same pattern as otherOrErrmethods in the struct. It properly handles cases where the edge is loaded or not loaded.Please verify that the index 3 used for
loadedTypes[3]is correct for thePrimaryImageedge. You can run the following command to check the order of edges in theNodeEdgesstruct:✅ Verification successful
Verified: Index for
PrimaryImageedge is correctThe
loadedTypes[3]correctly corresponds to thePrimaryImageedge in theNodeEdgesstruct.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash rg --type go "type NodeEdges struct {" -A 20 internal/ent/node.goLength of output: 177
Script:
#!/bin/bash rg --type go "type NodeEdges struct \{" -A 20 internal/ent/node.goLength of output: 1103
internal/ent/node/node.go (6)
37-38: LGTM: New constants for primary image relationshipThe addition of these constants is well-structured and consistent with the existing codebase. They provide the necessary definitions for the new primary image relationship.
Also applies to: 51-52, 82-88
137-137: LGTM: Added FieldPrimaryAssetID to Columns sliceThe addition of
FieldPrimaryAssetIDto theColumnsslice is correct and necessary for proper SQL operations involving the new field.
262-265: LGTM: New ordering function for primary_asset_idThe
ByPrimaryAssetIDfunction is correctly implemented, following the established pattern for ordering functions in this file.
305-310: LGTM: New ordering function for primary_image fieldThe
ByPrimaryImageFieldfunction is correctly implemented, consistent with other similar functions in the file. It properly utilizes thenewPrimaryImageStep()function for ordering.
409-415: LGTM: New SQL graph step function for primary imageThe
newPrimaryImageStepfunction is correctly implemented, following the established pattern for SQL graph step functions. It properly defines the Many-to-One relationship for the primary image.
Line range hint
1-458: Summary: Successful implementation of primary image relationshipThe changes in this file successfully implement the primary image relationship for the
Nodeentity. New constants, fields, and functions have been added consistently with the existing code structure. These additions enable querying and ordering by the primary image, which aligns with the PR objective of adding a primary asset cover image to library pages.However, to ensure completeness:
- Verify that corresponding changes have been made in related files (e.g., schema definitions, migrations).
- Confirm that the new relationship is properly handled in the business logic layer.
- Ensure that appropriate tests have been added or updated to cover this new functionality.
To verify the completeness of the implementation, please run the following script:
internal/ent/node_query.go (4)
36-36: LGTM: Addition ofwithPrimaryImagefieldThe addition of the
withPrimaryImagefield to theNodeQuerystruct is consistent with the PR objective of introducing a primary image relation for nodes. This field will be used for eager loading of the primary image asset.
542-551: LGTM: Implementation ofWithPrimaryImagemethodThe
WithPrimaryImagemethod is correctly implemented. It follows the established pattern for other relationships in theNodeQuerystruct and properly sets up thewithPrimaryImagefield with the provided options. This method will allow users to specify options for eager loading of primary images when constructing a query.
697-701: LGTM: Updates tosqlAllmethod for primary image handlingThe changes to the
sqlAllmethod correctly integrate the new primary image relationship into the existing query execution logic. TheloadedTypesarray is properly updated to include the new relationship, and the condition to load the primary image is added following the established pattern for other relationships. These changes ensure that the primary image will be eagerly loaded when requested.Also applies to: 750-755
1258-1260: LGTM: Update toquerySpecmethod for primary image handlingThe changes to the
querySpecmethod correctly add handling for thewithPrimaryImagefield. The new condition ensures that thePrimaryAssetIDcolumn is included in the query when the primary image relationship is being eagerly loaded. This implementation follows the established pattern for other relationships and is consistent with the overall design.internal/ent/node_create.go (4)
134-146: New methods for PrimaryAssetID look goodThe new methods
SetPrimaryAssetIDandSetNillablePrimaryAssetIDare correctly implemented and follow the existing patterns in the codebase. They provide a way to set the "primary_asset_id" field, which aligns with the PR objective of adding a primary asset cover image to library pages.
807-823: New upsert methods for PrimaryAssetID look goodThe new methods
SetPrimaryAssetID,UpdatePrimaryAssetID, andClearPrimaryAssetIDin the NodeUpsert struct are correctly implemented. They follow the existing patterns in the codebase and provide the necessary functionality for handling the "primary_asset_id" field during upsert operations. This addition aligns with the PR objective of adding support for a primary asset cover image.
1064-1083: New single-row upsert methods for PrimaryAssetID look goodThe new methods
SetPrimaryAssetID,UpdatePrimaryAssetID, andClearPrimaryAssetIDin the NodeUpsertOne struct are correctly implemented. They follow the existing patterns in the codebase and provide the necessary functionality for handling the "primary_asset_id" field during single-row upsert operations. This addition complements the bulk upsert methods and ensures consistent handling of the primary asset field across different operation types.
1499-1518: New bulk upsert methods for PrimaryAssetID look good and complete the implementationThe new methods
SetPrimaryAssetID,UpdatePrimaryAssetID, andClearPrimaryAssetIDin the NodeUpsertBulk struct are correctly implemented. They follow the existing patterns in the codebase and provide the necessary functionality for handling the "primary_asset_id" field during bulk upsert operations.Overall, these additions to the
node_create.gofile comprehensively implement the support for a primary asset (cover image) in the Node entity. The new methods cover all necessary operations: create, update, upsert (both single and bulk), and clear. This implementation aligns well with the PR objective of adding a primary asset cover image to library pages.The consistent implementation across different operation types (create, single upsert, bulk upsert) ensures that the primary asset can be handled correctly in various scenarios, providing a robust solution for managing cover images in the library pages.
internal/ent/mutation.go (22)
3765-3769: LGTM: New fields for parent-child asset relationship.The new fields
parent,clearedparent,assets,removedassets, andclearedassetsare correctly implemented to support the parent-child relationship for assets. This aligns with the PR objective of enhancing asset management.
4564-4564: LGTM: Updated Fields method.The
Fieldsmethod has been correctly updated to include 7 fields, which aligns with the new fields added to the struct.
4583-4585: LGTM: ParentAssetID added to Fields method.The
ParentAssetIDfield has been correctly added to the list of fields returned by theFieldsmethod. This is consistent with the new parent-child relationship implementation for assets.
4606-4607: LGTM: ParentAssetID case added to switch statement.The
asset.FieldParentAssetIDcase has been correctly added to the switch statement. This is consistent with the new parent-child relationship implementation for assets.
4629-4630: LGTM: ParentAssetID case added to switch statement.The
asset.FieldParentAssetIDcase has been correctly added to the switch statement for retrieving old field values. This is consistent with the new parent-child relationship implementation for assets.
4682-4688: LGTM: ParentAssetID case added to switch statement for setting field values.The
asset.FieldParentAssetIDcase has been correctly added to the switch statement for setting field values. The implementation includes proper type checking before setting the value, which is a good practice for maintaining data integrity.
4737-4739: LGTM: ParentAssetID added to ClearedFields method.The
ParentAssetIDfield has been correctly added to the list of cleared fields in theClearedFieldsmethod. This is consistent with the new parent-child relationship implementation for assets.
4757-4759: LGTM: ParentAssetID case added to switch statement for clearing nullable fields.The
asset.FieldParentAssetIDcase has been correctly added to the switch statement for clearing nullable fields. This is consistent with the new parent-child relationship implementation for assets.
4786-4795: LGTM: ParentAssetID case added to reset switch and AddedEdges method updated.The
asset.FieldParentAssetIDcase has been correctly added to the switch statement for resetting fields. Additionally, theAddedEdgesmethod has been updated to include 7 edges, which is consistent with the new relationships added to the asset struct.
4808-4813: LGTM: Parent and Assets edges added to AddedEdges method.The
ParentandAssetsedges have been correctly added to the list of added edges in theAddedEdgesmethod. This is consistent with the new parent-child relationship implementation for assets.
4846-4855: LGTM: Parent and Assets edge cases added to edge IDs switch statement.The
asset.EdgeParentandasset.EdgeAssetscases have been correctly added to the switch statement for edge IDs. The implementation includes proper null checks before returning the IDs, which is a good practice for preventing null pointer exceptions.
4868-4868: LGTM: RemovedEdges method updated.The
RemovedEdgesmethod has been correctly updated to include 7 edges, which is consistent with the new relationships added to the asset struct.
4878-4880: LGTM: Assets edge added to RemovedEdges method.The
Assetsedge has been correctly added to the list of removed edges in theRemovedEdgesmethod. This is consistent with the new parent-child relationship implementation for assets.
4909-4914: LGTM: Assets edge case added to removed edge IDs switch statement.The
asset.EdgeAssetscase has been correctly added to the switch statement for removed edge IDs. This is consistent with the new parent-child relationship implementation for assets.
4927-4927: LGTM: ClearedEdges method updated.The
ClearedEdgesmethod has been correctly updated to include 7 edges, which is consistent with the new relationships added to the asset struct.
4940-4945: LGTM: Parent and Assets edges added to ClearedEdges method.The
ParentandAssetsedges have been correctly added to the list of cleared edges in theClearedEdgesmethod. This is consistent with the new parent-child relationship implementation for assets.
4964-4967: LGTM: Parent and Assets edge cases added to edge clearing status switch statement.The
asset.EdgeParentandasset.EdgeAssetscases have been correctly added to the switch statement for edge clearing status. This is consistent with the new parent-child relationship implementation for assets.
4981-4983: LGTM: Parent edge case added to clear unique edges switch statement.The
asset.EdgeParentcase has been correctly added to the switch statement for clearing unique edges. This is consistent with the new parent-child relationship implementation for assets.
5004-5009: LGTM: Parent and Assets edge cases added to reset edges switch statement.The
asset.EdgeParentandasset.EdgeAssetscases have been correctly added to the switch statement for resetting edges. This is consistent with the new parent-child relationship implementation for assets.
14709-14710: LGTM: New fields for primary image added.The new fields
primary_imageandclearedprimary_imagehave been correctly added to the struct. This aligns with the PR objective of adding a primary asset cover image to library pages.
Line range hint
15844-16430: LGTM: Consistent updates for primary image field and relationship.The various methods and switch statements have been correctly updated to include the new primary image field and relationship. These changes are consistent throughout the file and follow the established patterns in the codebase. Proper null checks and error handling are included where necessary.
The updates include:
- Addition of primary image field to the
Fieldsmethod- Updates to
AddedEdges,RemovedEdges, andClearedEdgesmethods- New cases in switch statements for handling different operations on the primary image field and edge
These changes collectively support the PR objective of adding a primary asset cover image to library pages.
Line range hint
1-16430: Overall assessment: Well-implemented changes for asset relationships and primary images.The changes in this file consistently implement two main features:
- A parent-child relationship for assets
- A primary image for nodes
These implementations align well with the PR objective of adding a primary asset cover image to library pages. The code quality is good, with proper error handling and consistent implementation throughout the file. The new methods and fields follow the established patterns in the codebase, which will make it easier for developers familiar with the project to understand and work with these new features.
A few minor suggestions were made for improving documentation, but overall, the changes are well-structured and ready for integration.
app/transports/http/openapi/server_gen.go (13)
Line range hint
23-27: LGTM: Addition of nullable package importThe addition of the "github.com/oapi-codegen/nullable" import is appropriate and aligns with the need to handle nullable fields in OpenAPI specifications.
460-463: LGTM: Addition of Parent field to Asset structThe addition of the
Parent *Assetfield with theomitemptyJSON tag is well-implemented. This change introduces a self-referential structure for Assets, allowing for a parent-child relationship. This aligns with the PR objective of adding a recursive backreference to Assets for change versioning.
1453-1456: LGTM: Addition of Parent and PrimaryImage fields to Node structThe addition of
Parent *NodeandPrimaryImage *Assetfields withomitemptyJSON tags is well-implemented. These changes introduce a self-referential structure for Nodes and allow for a primary image association, directly aligning with the PR objective of adding a primary image (cover image) to library pages.
1493-1496: Please clarify the purpose of this duplicate codeThis code segment is identical to the previous one (lines 1453-1456). Could you please clarify if this duplication is intentional? If it's defining the same fields for different structs or interfaces, it might be beneficial to add a comment explaining the purpose of the duplication.
1526-1530: LGTM: Addition of PrimaryImageAssetId fieldThe addition of the
PrimaryImageAssetId *AssetIDfield with theomitemptyJSON tag is well-implemented. This change is consistent with the addition of the PrimaryImage field in the Node struct and is appropriate for update operations. The use of AssetID instead of the full Asset struct is a good choice for this context.
1568-1572: LGTM: Addition of nullable PrimaryImageAssetId fieldThe addition of the
PrimaryImageAssetId nullable.Nullable[NullableIdentifier]field with theomitemptyJSON tag is well-implemented. This change is consistent with the previous additions related to primary images and provides enhanced flexibility in handling the primary image asset ID. The use ofnullable.Nullableallows for explicit representation of null values, which can be beneficial in certain API scenarios.
1690-1694: LGTM: Addition of NullableIdentifier typeThe addition of the
NullableIdentifiertype as an alias forstringis a good implementation. This new type, likely used in conjunction with thenullable.Nullabletype, provides a clear way to represent identifiers that can be null. The implementation as a string alias is simple and effective.
2672-2676: LGTM: Addition of ParentAssetIDQuery typeThe addition of the
ParentAssetIDQuerytype as an alias forstringis a good implementation. This new type is likely used for querying parent asset IDs, supporting the new parent-child relationship for assets introduced earlier. The implementation as a string alias is simple and consistent with other ID types in the codebase.
3060-3068: LGTM: Well-documented addition of ParentAssetId fieldThe addition of the
ParentAssetId *ParentAssetIDQueryfield is well-implemented and thoroughly documented. The detailed comment provides clear guidance on how to use this field for uploading new versions of existing assets, supporting the new parent-child relationship for assets. The use of a pointer and omitempty tags in both form and JSON tags is appropriate for an optional field.
6161-6176: LGTM: Proper handling of new ParentAssetId parameterThe addition of logic to handle the new
ParentAssetIdparameter is well-implemented. The code follows the existing patterns for parameter handling, including proper nil checks and error handling. The query parameter building process is consistent with other parameters in the function.
18168-18176: LGTM: Proper binding of new ParentAssetId query parameterThe addition of logic to bind the new
ParentAssetIdquery parameter is well-implemented. The code follows the existing patterns for parameter binding, using the appropriate binding function for query parameters. The error handling and error message formatting are consistent with other parameter bindings in the function.
27628-27944: LGTM: Updated Swagger specificationThe Swagger specification has been updated, which is crucial for maintaining accurate API documentation. This update likely reflects all the changes made to the API in this PR.
However, as the content is encoded, it's challenging to verify the exact changes in this review. It would be beneficial to manually review the decoded Swagger spec to ensure all API changes are accurately represented.
1626-1628:⚠️ Potential issueLGTM: PrimaryImage addition, but there's a typo nearby
The addition of the
PrimaryImage *Assetfield is consistent with the PR's objective and earlier changes.However, there's a typo in the nearby
Recomentationsfield. It should be corrected:- Recomentations DatagraphItemList `json:"recomentations"` + Recommendations DatagraphItemList `json:"recommendations"`Likely invalid or redundant comment.
app/resources/asset/asset.go (3)
22-28: Addition ofAddVersionmethod is well-structured.The new
AddVersionmethod in theRepositoryinterface extends the asset functionality appropriately and follows the existing code conventions.
61-61: Confirm the assignment of theParentfield inAssetstruct.Assigning
Parent: parent,in theFromModelfunction ensures theParentfield is correctly initialized. Verify thatparentaccurately reflects the presence or absence of a parent asset.
46-46:⚠️ Potential issueManage recursive serialization due to the
Parentfield.Introducing
Parent opt.Optional[Asset]to theAssetstruct creates a recursive type. Ensure that serialization (e.g., JSON marshaling) handles theParentfield properly to prevent infinite recursion or stack overflow errors.Run the following script to identify instances where
Assetis serialized without handling theParentfield:internal/deletable/deletable.go (2)
49-51: FunctionSkip[T any]looks goodThe implementation correctly provides an opt-out mechanism for cases where the inbound type is not a nullable type. It effectively handles the optional value without introducing unnecessary complexity.
53-55: MethodGet()is properly implementedThe
Getmethod accurately returns the optional value and thedeleteflag, allowing callers to determine the state of theValue[T]instance.app/services/asset/asset_upload/uploader.go (1)
52-52: LGTM! Addition of 'ParentID' field to 'Options' structThe inclusion of
ParentIDas an optional field allows the uploader to handle versioning, aligning with the PR objective to support asset versioning.web/src/screens/library/LibraryPageScreen/useLibraryPageScreen.ts (12)
10-10: Avoid importing from internal modules; use the public API to importAbstractCropperRef.Importing directly from internal paths like
'react-advanced-cropper/dist/components/AbstractCropper'can lead to fragility in the codebase, as internal structures can change without notice. Instead, ifAbstractCropperRefis exported from the main module, you should import it directly from'react-advanced-cropper'or use the recommended public API as per the library's documentation.
37-43: LGTM!The
CoverImageFormSchemais correctly defined using Zod to validate the cover image data. It allows either aCoverImageSchemaobject or an object with anasset_idstring field.
54-54: LGTM!The
FormSchemais updated to include an optionalcoverImagefield of typeCoverImageFormSchema. This allows the form to handle cover image data when submitted.
74-75: LGTM!The
cropperRefis correctly defined usinguseRefwith a type ofFixedCropperRef. This ref will be used to access the cropper instance and its methods.
113-119: LGTM!The
primaryAssetEditingURLis correctly derived using thegetAssetURLfunction. It uses theparentasset path if available, otherwise falls back to theprimary_imagepath. This ensures that the original image is used for editing.
121-121: LGTM!The
primaryAssetURLis correctly derived using thegetAssetURLfunction with theprimary_imagepath. This URL represents the current cover image being displayed.
123-123: LGTM!The
initialCoverCoordinatesare correctly parsed from the node's metadata using theparseNodeMetadatafunction. This provides the initial crop coordinates for the cover image.
149-209: LGTM!The
uploadCroppedCoverfunction is implemented correctly:
- It checks if the
cropperRefis available and throws an error if not.- It retrieves the canvas from the cropper and throws an error if not available.
- It gets the crop coordinates from the cropper and throws an error if not available.
- It converts the canvas to a Blob using
toBlobmethod.- It determines the
parent_asset_idbased on whether the current primary image is a copy or not.- It uploads the cropped image using
assetUploadfunction with the appropriateparent_asset_id.- It returns an object with the replacement status, crop coordinates, and the uploaded asset.
214-214: LGTM!The
handleSubmitfunction is updated to call theuploadCroppedCoverfunction and await its result before updating the node. This ensures that the cropped cover image is uploaded and associated with the node.
303-306: LGTM!The
useLibraryPageScreenhook correctly returns the necessary properties:
cropperRef: The ref to access the cropper instance.primaryAssetURL: The URL of the current cover image being displayed.primaryAssetEditingURL: The URL of the original image used for editing.initialCoverCoordinates: The initial crop coordinates for the cover image.
156-157: Use Error objects when rejecting promises for better error handling.When rejecting a Promise, it's advisable to use an
Errorobject instead of a string. This provides better stack traces and aids in debugging.Apply this diff to improve error handling:
- reject("An unexpected error occurred with the image editor."); + reject(new Error("An unexpected error occurred with the image editor."));
170-172: Use Error objects when rejecting promises for better error handling.When rejecting a Promise, it's advisable to use an
Errorobject instead of a string. This provides better stack traces and aids in debugging.Apply this diff to improve error handling:
- reject("An unexpected error occurred with the image editor."); + reject(new Error("An unexpected error occurred with the image editor."));internal/ent/asset.go (10)
35-36: Addition of 'ParentAssetID' field to Asset structThe new field
ParentAssetID *xid.IDcorrectly introduces a parent-child relationship in theAssetstruct, allowing assets to reference a parent asset. This change is well-implemented and follows the existing coding patterns.
53-56: Inclusion of 'Parent' and 'Assets' in AssetEdgesAdding
ParentandAssetstoAssetEdgesenables traversal of parent and child assets within the entity graph. This enhances the navigability and relational integrity of assets.
61-61: Verify 'loadedTypes' array indices after adding new edgesThe
loadedTypesarray size has increased to[7]boolto accommodate the new edges. Please verify that all indices used in edge loading and checking (e.g., inParentOrErr,AssetsOrErr,EventOrErr) correctly correspond to their respective edges to prevent any misalignment issues.
102-111: Confirm correct index usage in 'ParentOrErr' methodIn the
ParentOrErrmethod, the indexe.loadedTypes[4]is used to check if the 'parent' edge has been loaded. Ensure that this index correctly corresponds to the 'parent' edge in the updatedloadedTypesarray after adding new edges.
113-120: Confirm correct index usage in 'AssetsOrErr' methodThe
AssetsOrErrmethod uses the indexe.loadedTypes[5]to verify if the 'assets' edge is loaded. Please confirm that this index aligns with the 'assets' edge in theloadedTypesarray to ensure proper edge loading.
125-127: Ensure updated index in 'EventOrErr' method corresponds to 'event' edgeThe
EventOrErrmethod now referencese.loadedTypes[6]for the 'event' edge. Verify that this index matches the 'event' edge in the updatedloadedTypesarray to maintain accurate edge loading behavior.
136-137: Proper handling of 'ParentAssetID' in 'scanValues'The addition of
asset.FieldParentAssetIDin thescanValuesfunction ensures that theParentAssetIDfield is correctly scanned from SQL rows. The use ofsql.NullScanneraccommodates potential null values, which is appropriate for an optional field.
207-213: Correct assignment of 'ParentAssetID' in 'assignValues'In the
assignValuesmethod, the assignment logic forParentAssetIDproperly checks for null values and assigns the scannedxid.IDwhen valid. This implementation handles the optional nature ofParentAssetIDeffectively.
247-250: Addition of 'QueryParent' method enhances parent asset retrievalThe
QueryParentmethod allows for querying the 'parent' edge of anAssetentity. This addition is consistent with existing query methods and facilitates easy retrieval of a parent asset.
252-255: Introduction of 'QueryAssets' method for accessing child assetsThe
QueryAssetsmethod enables querying the 'assets' edge to retrieve child assets associated with a parent asset. This method is implemented following the established patterns in the codebase.internal/ent/asset/asset.go (10)
30-31: Addition ofFieldParentAssetIDconstantThe constant
FieldParentAssetIDis correctly defined to represent theparent_asset_idfield in the database.
40-43: Addition ofEdgeParentandEdgeAssetsconstantsThe constants
EdgeParentandEdgeAssetsare appropriately added to represent the parent and assets edges in mutations.
70-77: Definition of parent and assets table and column constantsThe constants
ParentTable,ParentColumn,AssetsTable, andAssetsColumnare correctly defined for the self-referential relationship in the assets table.
96-96: IncludingFieldParentAssetIDinColumnssliceThe
FieldParentAssetIDis properly added to theColumnsslice for validation and query purposes.
167-170: Addition ofByParentAssetIDordering functionThe function
ByParentAssetIDallows ordering results by theparent_asset_idfield. The implementation is correct and follows existing patterns.
221-226: Addition ofByParentFieldordering functionThe function
ByParentFieldenables ordering the results by a specified field of the parent asset. It is correctly implemented usingnewParentStep().
228-233: Addition ofByAssetsCountordering functionThe function
ByAssetsCountfacilitates ordering by the count of child assets. The implementation is accurate.
235-240: Addition ofByAssetsordering functionThe function
ByAssetsenables ordering results based on terms related to the assets edge. The function is correctly implemented and aligns with existing code.
283-289: Definition ofnewParentStepfunctionThe
newParentStepfunction defines the SQL graph step for the parent edge. The self-referential many-to-one edge is correctly established.
290-296: Definition ofnewAssetsStepfunctionThe
newAssetsStepfunction defines the SQL graph step for the assets edge. The self-referential one-to-many edge is accurately set up.internal/ent/asset/where.go (12)
84-87: Addition ofParentAssetIDpredicate functionThe function
ParentAssetIDhas been correctly added, mirroring the existing pattern for equality predicates on fields. This enhances usability and maintains consistency across the codebase.
354-358: Implementation ofParentAssetIDEQequality predicateThe
ParentAssetIDEQfunction properly implements the equality predicate for theparent_asset_idfield, allowing precise queries whereparent_asset_idequals a specific value.
359-363: Implementation ofParentAssetIDNEQinequality predicateThe
ParentAssetIDNEQfunction correctly provides the not-equal predicate for theparent_asset_idfield, enabling queries for assets whereparent_asset_iddoes not equal a specific value.
364-368: Addition ofParentAssetIDInpredicateThe
ParentAssetIDInfunction is appropriately implemented, allowing querying assets whoseparent_asset_idis in a set of specified IDs.
369-373: Addition ofParentAssetIDNotInpredicateThe
ParentAssetIDNotInfunction correctly implements the not-in predicate for theparent_asset_idfield, enabling the selection of assets whoseparent_asset_idis not within a specified set.
374-382: Implementation of comparison predicates forParentAssetIDThe functions
ParentAssetIDGT,ParentAssetIDGTE,ParentAssetIDLT, andParentAssetIDLTEare correctly added, providing greater-than, greater-than-or-equal, less-than, and less-than-or-equal predicates for theparent_asset_idfield. These predicates enhance querying capabilities, although their practical use with ID fields may be limited.
394-432: Inclusion of string predicates forParentAssetIDfieldThe functions
ParentAssetIDContains,ParentAssetIDHasPrefix,ParentAssetIDHasSuffix,ParentAssetIDEqualFold, andParentAssetIDContainsFoldare added for theparent_asset_idfield, providing string-based predicates. While IDs are typically unique identifiers, including these predicates can offer additional flexibility for querying, especially if the IDs have meaningful prefixes or patterns.
412-421: Null checking predicates forParentAssetIDThe functions
ParentAssetIDIsNilandParentAssetIDNotNilare correctly implemented, allowing queries to check for the presence or absence of aparent_asset_id. This is essential when dealing with optional relationships or identifying root assets without parents.
526-535: Correct addition ofHasParentedge predicateThe
HasParentfunction is properly implemented to apply the has-edge predicate on the "parent" edge, enabling queries that select assets which have a parent asset.
537-547: Implementation ofHasParentWithedge predicateThe
HasParentWithfunction correctly applies the has-edge predicate with additional conditions, allowing for more refined queries involving the "parent" edge based on specified predicates.
549-558: Addition ofHasAssetsedge predicateThe
HasAssetsfunction is properly added, enabling queries to select assets that have associated child assets, supporting hierarchical data retrieval.
560-570: Implementation ofHasAssetsWithedge predicateThe
HasAssetsWithfunction is correctly implemented, allowing for queries on assets with specific child assets that meet given conditions. This enhances the ability to perform complex queries within the asset hierarchy.internal/ent/asset_create.go (10)
84-88: Implementation ofSetParentAssetIDmethod is correctThe
SetParentAssetIDmethod is correctly implemented and follows the existing patterns in the codebase.
90-96: Implementation ofSetNillableParentAssetIDmethod is correctThe
SetNillableParentAssetIDmethod properly handles nil values and is consistent with the codebase conventions.
168-172: Implementation ofSetParentIDmethod is correctThe
SetParentIDmethod correctly sets the "parent" edge to the Asset entity by ID.
174-180: Implementation ofSetNillableParentIDmethod is correctThe
SetNillableParentIDmethod appropriately handles nil values when setting the "parent" edge.
182-185: Implementation ofSetParentmethod is correctThe
SetParentmethod correctly sets the "parent" edge to the Asset entity.
187-191: Implementation ofAddAssetIDsmethod is correctThe
AddAssetIDsmethod correctly adds the "assets" edge to the Asset entity by IDs.
193-200: Implementation ofAddAssetsmethod is correctThe
AddAssetsmethod properly adds the "assets" edges to the Asset entity.
585-602: Implementation ofAssetUpsertmethods forparent_asset_idis correctThe
SetParentAssetID,UpdateParentAssetID, andClearParentAssetIDmethods in theAssetUpsertstruct are correctly implemented and follow the existing patterns.
738-758: Implementation ofAssetUpsertOnemethods forparent_asset_idis correctThe methods
SetParentAssetID,UpdateParentAssetID, andClearParentAssetIDinAssetUpsertOneare correctly implemented and align with the patterns used in other upsert operations.
1061-1081: Implementation ofAssetUpsertBulkmethods forparent_asset_idis correctThe bulk upsert methods
SetParentAssetID,UpdateParentAssetID, andClearParentAssetIDare correctly implemented for handling theparent_asset_idfield.internal/ent/asset_query.go (11)
36-37: Adding fields for new relationshipsThe addition of
withParentandwithAssetsfields to theAssetQuerystruct correctly enables querying parent and child assets.
164-184: Implementation ofQueryParentmethodThe
QueryParentmethod is properly implemented to facilitate querying the parent asset associated with the current asset, following consistent patterns used in other query methods.
186-206: Implementation ofQueryAssetsmethodThe
QueryAssetsmethod is correctly added to enable querying child assets linked to the current asset, maintaining consistency with existing query methods.
426-427: Including new fields inClonemethodBy adding
withParentandwithAssetsto theClonemethod, the query builder ensures that these configurations are duplicated correctly during cloning.
480-489: AddingWithParentmethod for eager loadingThe
WithParentmethod is appropriately implemented to allow eager loading of the parent asset, consistent with the existing edge-loading methods.
491-500: AddingWithAssetsmethod for eager loadingThe
WithAssetsmethod is correctly added to enable eager loading of child assets, following the established pattern for edge-loading methods.
591-597: UpdatingloadedTypesarrayThe
loadedTypesarray now includeswithParentandwithAssets, correctly tracking the loading state of the new relationships.
649-661: Handling eager loading for new edgesThe code effectively handles the eager loading of
withParentandwithAssets, ensuring related assets are loaded when these edges are queried.
884-915: ImplementingloadParentmethodThe
loadParentfunction is correctly implemented to load parent assets and assign them to their respective nodes, with appropriate error handling for missing foreign keys.
916-948: ImplementingloadAssetsmethodThe
loadAssetsfunction properly loads child assets and assigns them to their parent assets, efficiently handling the relationships and potential nil references.
1012-1014: IncludingParentAssetIDin query specificationsBy adding
ParentAssetIDto the node columns whenwithParentis used, the code ensures that necessary fields are included in the query for loading parent assets.internal/ent/node/where.go (3)
104-107: Addition ofPrimaryAssetIDpredicate function is consistent and correctThe
PrimaryAssetIDfunction correctly applies an equality check on theprimary_asset_idfield, mirroring the pattern used for other fields in the codebase.
674-753: NewPrimaryAssetIDpredicate functions are properly implementedThe suite of predicate functions for
PrimaryAssetID—includingPrimaryAssetIDEQ,PrimaryAssetIDNEQ,PrimaryAssetIDIn, and others—are correctly added and follow the established conventions. The conversion ofxid.IDto string in functions likePrimaryAssetIDContainsis consistent with similar implementations elsewhere in the code.
933-954:HasPrimaryImageedge predicates are correctly definedThe
HasPrimaryImageandHasPrimaryImageWithfunctions properly implement the predicates for theprimary_imageedge. The use ofsqlgraph.NewStepand the edge configuration aligns with existing edge predicates, ensuring consistent behavior in querying relationships.internal/ent/asset_update.go (1)
187-193:⚠️ Potential issueRedundant assignment in
SetNillableParentIDmethodIn the
SetNillableParentIDmethod, the assignmentau = au.SetParentID(*id)is redundant sinceSetParentIDalready returnsau. You can remove the assignment and simply callau.SetParentID(*id).Apply this diff to remove the redundant assignment:
func (au *AssetUpdate) SetNillableParentID(id *xid.ID) *AssetUpdate { if id != nil { - au = au.SetParentID(*id) + au.SetParentID(*id) } return au }Likely invalid or redundant comment.
internal/ent/client.go (3)
1332-1346: Excellent addition of theQueryParentmethod to theAssetClient!This new method allows querying the parent asset of a given
Asset, enabling traversal of the parent-child relationship between assets. It utilizes thesqlgraphpackage to define the necessary database join step.
1348-1362: Great work on adding theQueryAssetsmethod to theAssetClient!This method enables querying the child assets associated with a given
Asset, facilitating the retrieval of related assets in a one-to-many relationship. It leverages thesqlgraphpackage to define the appropriate database join step.
3571-3585: Fantastic addition of theQueryPrimaryImagemethod to theNodeClient!This new method allows querying the primary image associated with a given
Node, enabling easy retrieval of the node's main visual representation. It utilizes thesqlgraphpackage to define the necessary database join step between thenodeandassettables.
| export const CoverImageSchema = z.object({ | ||
| top: z.number(), | ||
| left: z.number(), | ||
| }); | ||
| export type CoverImage = z.infer<typeof CoverImageSchema>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding constraints to 'top' and 'left' values.
The current schema allows any number for 'top' and 'left'. Consider adding constraints to ensure these values are within a valid range (e.g., percentages between 0 and 100).
Example:
export const CoverImageSchema = z.object({
top: z.number().min(0).max(100),
left: z.number().min(0).max(100),
});This would prevent invalid positions and improve data integrity.
| export function parseNodeMetadata( | ||
| metadata: Metadata | undefined, | ||
| ): NodeMetadata { | ||
| const parsed = NodeMetadataSchema.safeParse(metadata); | ||
| if (parsed.success) { | ||
| return parsed.data; | ||
| } | ||
|
|
||
| return {}; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error logging and improving error handling.
While the function safely handles parsing failures by returning an empty object, this approach might hide important information about why the parsing failed. Consider the following improvements:
- Add error logging when parsing fails to help with debugging.
- Return a more informative result, such as a union type that includes both successful and error cases.
Example:
import { logger } from "@/utils/logger"; // Assume we have a logger utility
export function parseNodeMetadata(
metadata: Metadata | undefined
): { success: true; data: NodeMetadata } | { success: false; error: z.ZodError } {
const parsed = NodeMetadataSchema.safeParse(metadata);
if (parsed.success) {
return { success: true, data: parsed.data };
}
logger.warn("Failed to parse node metadata", parsed.error);
return { success: false, error: parsed.error };
}This approach provides more context when parsing fails, aiding in debugging and allowing callers to handle errors more gracefully.
| "use client"; | ||
|
|
||
| import type { Assign } from "@ark-ui/react"; | ||
| import { FileUpload } from "@ark-ui/react/file-upload"; | ||
|
|
||
| import { | ||
| type FileUploadVariantProps, | ||
| fileUpload, | ||
| } from "@/styled-system/recipes"; | ||
| import type { ComponentProps, HTMLStyledProps } from "@/styled-system/types"; | ||
| import { createStyleContext } from "@/utils/create-style-context"; | ||
|
|
||
| const { withProvider, withContext } = createStyleContext(fileUpload); | ||
|
|
||
| export type RootProviderProps = ComponentProps<typeof RootProvider>; | ||
| export const RootProvider = withProvider< | ||
| HTMLDivElement, | ||
| Assign< | ||
| Assign<HTMLStyledProps<"div">, FileUpload.RootProviderBaseProps>, | ||
| FileUploadVariantProps | ||
| > | ||
| >(FileUpload.RootProvider, "root"); | ||
|
|
||
| export type RootProps = ComponentProps<typeof Root>; | ||
| export const Root = withProvider< | ||
| HTMLDivElement, | ||
| Assign< | ||
| Assign<HTMLStyledProps<"div">, FileUpload.RootBaseProps>, | ||
| FileUploadVariantProps | ||
| > | ||
| >(FileUpload.Root, "root"); | ||
|
|
||
| export const Dropzone = withContext< | ||
| HTMLDivElement, | ||
| Assign<HTMLStyledProps<"div">, FileUpload.DropzoneBaseProps> | ||
| >(FileUpload.Dropzone, "dropzone"); | ||
|
|
||
| export const ItemDeleteTrigger = withContext< | ||
| HTMLButtonElement, | ||
| Assign<HTMLStyledProps<"button">, FileUpload.ItemDeleteTriggerBaseProps> | ||
| >(FileUpload.ItemDeleteTrigger, "itemDeleteTrigger"); | ||
|
|
||
| export const ItemGroup = withContext< | ||
| HTMLUListElement, | ||
| Assign<HTMLStyledProps<"ul">, FileUpload.ItemGroupBaseProps> | ||
| >(FileUpload.ItemGroup, "itemGroup"); | ||
|
|
||
| export const ItemName = withContext< | ||
| HTMLDivElement, | ||
| Assign<HTMLStyledProps<"div">, FileUpload.ItemNameBaseProps> | ||
| >(FileUpload.ItemName, "itemName"); | ||
|
|
||
| export const ItemPreviewImage = withContext< | ||
| HTMLImageElement, | ||
| Assign<HTMLStyledProps<"img">, FileUpload.ItemPreviewImageBaseProps> | ||
| >(FileUpload.ItemPreviewImage, "itemPreviewImage"); | ||
|
|
||
| export const ItemPreview = withContext< | ||
| HTMLImageElement, | ||
| Assign<HTMLStyledProps<"div">, FileUpload.ItemPreviewBaseProps> | ||
| >(FileUpload.ItemPreview, "itemPreview"); | ||
|
|
||
| export const Item = withContext< | ||
| HTMLLIElement, | ||
| Assign<HTMLStyledProps<"li">, FileUpload.ItemBaseProps> | ||
| >(FileUpload.Item, "item"); | ||
|
|
||
| export const ItemSizeText = withContext< | ||
| HTMLDivElement, | ||
| Assign<HTMLStyledProps<"div">, FileUpload.ItemSizeTextBaseProps> | ||
| >(FileUpload.ItemSizeText, "itemSizeText"); | ||
|
|
||
| export const Label = withContext< | ||
| HTMLLabelElement, | ||
| Assign<HTMLStyledProps<"label">, FileUpload.LabelBaseProps> | ||
| >(FileUpload.Label, "label"); | ||
|
|
||
| export const Trigger = withContext< | ||
| HTMLButtonElement, | ||
| Assign<HTMLStyledProps<"button">, FileUpload.TriggerBaseProps> | ||
| >(FileUpload.Trigger, "trigger"); | ||
|
|
||
| export { | ||
| FileUploadContext as Context, | ||
| FileUploadHiddenInput as HiddenInput, | ||
| } from "@ark-ui/react/file-upload"; |
There was a problem hiding this comment.
Overall improvements with one remaining issue
This refactoring significantly enhances the file-upload.tsx component:
- Improved type safety and consistency across all components.
- Standardized styling approach using
withContextandwithProvider. - Clear and flexible exports for better usability.
- Enhanced modularity and maintainability.
These changes demonstrate a well-thought-out architecture and will likely improve developer experience when working with this component.
However, there's one remaining issue to address:
- The
ItemPreviewcomponent still has an inconsistency between its HTML element type and props type (lines 58-61).
Once this issue is resolved, the refactoring will be complete and highly effective.
| @@ -99,6 +102,7 @@ func serialiseAsset(a asset.Asset) openapi.Asset { | |||
| Id: a.ID.String(), | |||
| Filename: a.Name.String(), | |||
| Path: path, | |||
| Parent: opt.Map(a.Parent, serialiseAsset).Ptr(), | |||
There was a problem hiding this comment.
💡 Codebase verification
Potential Circular References Detected in Serialization
During verification, multiple structs were found with Parent fields that reference the same struct type. This could lead to infinite recursion during serialization if not properly handled.
internal/ent/node.go:62: Parent *Nodeinternal/ent/asset.go:54: Parent *Assetapp/transports/http/openapi/server_gen.go:461: Parent *Assetapp/transports/http/openapi/server_gen.go:1453: Parent *Nodeapp/transports/http/openapi/server_gen.go:1625: Parent *Nodeapp/transports/http/openapi/server_gen.go:1534: Parent *NodeSlugapp/transports/http/openapi/server_gen.go:1574: Slug *NodeSlugapp/transports/http/openapi/server_gen.go:1607: Parent *LinkReference
Please implement safeguards such as setting a maximum serialization depth or ensuring proper termination conditions to prevent potential infinite recursion and stack overflow errors.
🔗 Analysis chain
LGTM! Consider adding error handling for recursive serialization.
The addition of the Parent field to the openapi.Asset struct in the serialiseAsset function is consistent with the PR objectives. It allows including the parent asset information in the API response, enhancing asset relationships.
While the current implementation looks good, it's worth considering potential edge cases. Let's verify if there's any risk of infinite recursion:
If the script returns any matches, consider adding a depth limit to prevent potential stack overflow errors in case of circular references.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential circular references in asset relationships
# Test: Search for any circular references in asset relationships
rg --type go -e "Parent.*Parent" -e "ParentID.*ParentID"
Length of output: 2584
Script:
#!/bin/bash
# Description: Identify struct definitions with Parent fields referencing the same struct to detect potential circular references
ast-grep --lang go --pattern '
type $_ struct {
$$$
Parent *$_
$$$
}'
Length of output: 20904
| async function handleFileReject({ files }: FileUploadFileRejectDetails) { | ||
| if (files.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const file = files[0]; | ||
| if (!file) { | ||
| console.error( | ||
| "handleFileReject: files list non-empty but first file is falsy", | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| const accepted = acceptedMIMEs.reduce((prev: string[], curr: string) => { | ||
| const extensions = mime[curr]?.extensions; | ||
| if (!extensions) { | ||
| return prev; | ||
| } | ||
|
|
||
| return [...prev, ...extensions]; | ||
| }, []); | ||
|
|
||
| const acceptedList = accepted.map((e) => `.${e}`).join(", "); | ||
|
|
||
| // Vast majority of the time, there will only be one error, but join anyway. | ||
| const errorMessage = file.errors | ||
| .map((error) => { | ||
| switch (error) { | ||
| case "FILE_INVALID": | ||
| return "Invalid file."; | ||
| case "FILE_TOO_LARGE": | ||
| return "File is too large."; | ||
| case "FILE_INVALID_TYPE": | ||
| return `File must be of type ${acceptedList}`; | ||
| case "FILE_TOO_SMALL": | ||
| return "File is too small."; | ||
| case "TOO_MANY_FILES": | ||
| return "Too many files."; | ||
| default: | ||
| return "An unexpected error occurred while reading the file."; | ||
| } | ||
| }) | ||
| .join(", "); | ||
|
|
||
| toast.error(errorMessage); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve performance and refactor for better maintainability.
The handleFileReject function provides comprehensive error handling, but there are opportunities for improvement:
- Performance issue: The reduce function on lines 62-69 uses spread syntax, which can lead to O(n²) time complexity.
- The function is quite long and could benefit from refactoring for better readability and maintainability.
To address the performance issue, replace the spread syntax with push:
const accepted = acceptedMIMEs.reduce((prev: string[], curr: string) => {
const extensions = mime[curr]?.extensions;
if (!extensions) {
return prev;
}
- return [...prev, ...extensions];
+ prev.push(...extensions);
+ return prev;
}, []);Consider refactoring the error message generation into a separate function for better maintainability:
function getErrorMessage(error: string, acceptedList: string): string {
switch (error) {
case "FILE_INVALID":
return "Invalid file.";
case "FILE_TOO_LARGE":
return "File is too large.";
case "FILE_INVALID_TYPE":
return `File must be of type ${acceptedList}`;
case "FILE_TOO_SMALL":
return "File is too small.";
case "TOO_MANY_FILES":
return "Too many files.";
default:
return "An unexpected error occurred while reading the file.";
}
}Then use it in the handleFileReject function:
const errorMessage = file.errors
.map((error) => getErrorMessage(error, acceptedList))
.join(", ");This refactoring will make the function more concise and easier to maintain.
🧰 Tools
🪛 Biome
[error] 68-68: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
| builder.WriteString(", ") | ||
| if v := a.ParentAssetID; v != nil { | ||
| builder.WriteString("parent_asset_id=") | ||
| builder.WriteString(fmt.Sprintf("%v", *v)) | ||
| } |
There was a problem hiding this comment.
Fix formatting issue in 'String' method when 'ParentAssetID' is nil
The String method unconditionally writes a comma before checking if ParentAssetID is non-nil. If ParentAssetID is nil, this results in an extra comma, leading to inconsistent formatting. Consider adjusting the code to add the comma only when ParentAssetID is not nil.
Apply this diff to conditionally add the comma:
builder.WriteString(", ")
-if v := a.ParentAssetID; v != nil {
+if v := a.ParentAssetID; v != nil {
+ builder.WriteString(", ")
builder.WriteString("parent_asset_id=")
builder.WriteString(fmt.Sprintf("%v", *v))
}This change ensures that the comma is only included when ParentAssetID is present, maintaining correct formatting in the string representation.
Committable suggestion was skipped due to low confidence.
| if nodes := ac.mutation.ParentIDs(); len(nodes) > 0 { | ||
| edge := &sqlgraph.EdgeSpec{ | ||
| Rel: sqlgraph.M2O, | ||
| Inverse: true, | ||
| Table: asset.ParentTable, | ||
| Columns: []string{asset.ParentColumn}, | ||
| Bidi: false, | ||
| Target: &sqlgraph.EdgeTarget{ | ||
| IDSpec: sqlgraph.NewFieldSpec(asset.FieldID, field.TypeString), | ||
| }, | ||
| } | ||
| for _, k := range nodes { | ||
| edge.Target.Nodes = append(edge.Target.Nodes, k) | ||
| } | ||
| _node.ParentAssetID = &nodes[0] | ||
| _spec.Edges = append(_spec.Edges, edge) | ||
| } | ||
| if nodes := ac.mutation.AssetsIDs(); len(nodes) > 0 { |
There was a problem hiding this comment.
Validate that only one Parent ID is provided in AssetCreate mutation
In the AssetCreate method (lines 412-429), when setting up the parent edge, nodes := ac.mutation.ParentIDs() may contain multiple IDs. Since an asset should have only one parent, it's important to ensure that only one Parent ID is provided. Consider adding a validation check to enforce this constraint.
Apply this diff to add the validation:
if nodes := ac.mutation.ParentIDs(); len(nodes) > 0 {
+ if len(nodes) != 1 {
+ return nil, errors.New("Asset can have only one parent")
+ }
edge := &sqlgraph.EdgeSpec{
Rel: sqlgraph.M2O,
Inverse: true,
Table: asset.ParentTable,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if nodes := ac.mutation.ParentIDs(); len(nodes) > 0 { | |
| edge := &sqlgraph.EdgeSpec{ | |
| Rel: sqlgraph.M2O, | |
| Inverse: true, | |
| Table: asset.ParentTable, | |
| Columns: []string{asset.ParentColumn}, | |
| Bidi: false, | |
| Target: &sqlgraph.EdgeTarget{ | |
| IDSpec: sqlgraph.NewFieldSpec(asset.FieldID, field.TypeString), | |
| }, | |
| } | |
| for _, k := range nodes { | |
| edge.Target.Nodes = append(edge.Target.Nodes, k) | |
| } | |
| _node.ParentAssetID = &nodes[0] | |
| _spec.Edges = append(_spec.Edges, edge) | |
| } | |
| if nodes := ac.mutation.AssetsIDs(); len(nodes) > 0 { | |
| if nodes := ac.mutation.ParentIDs(); len(nodes) > 0 { | |
| if len(nodes) != 1 { | |
| return nil, errors.New("Asset can have only one parent") | |
| } | |
| edge := &sqlgraph.EdgeSpec{ | |
| Rel: sqlgraph.M2O, | |
| Inverse: true, | |
| Table: asset.ParentTable, | |
| Columns: []string{asset.ParentColumn}, | |
| Bidi: false, | |
| Target: &sqlgraph.EdgeTarget{ | |
| IDSpec: sqlgraph.NewFieldSpec(asset.FieldID, field.TypeString), | |
| }, | |
| } | |
| for _, k := range nodes { | |
| edge.Target.Nodes = append(edge.Target.Nodes, k) | |
| } | |
| _node.ParentAssetID = &nodes[0] | |
| _spec.Edges = append(_spec.Edges, edge) | |
| } | |
| if nodes := ac.mutation.AssetsIDs(); len(nodes) > 0 { |
| func (auo *AssetUpdateOne) SetNillableParentID(id *xid.ID) *AssetUpdateOne { | ||
| if id != nil { | ||
| auo = auo.SetParentID(*id) | ||
| } | ||
| return auo | ||
| } |
There was a problem hiding this comment.
Redundant assignment in SetNillableParentID method
In the SetNillableParentID method of AssetUpdateOne, the assignment auo = auo.SetParentID(*id) is redundant since SetParentID already returns auo. You can remove the assignment and simply call auo.SetParentID(*id).
Apply this diff to remove the redundant assignment:
func (auo *AssetUpdateOne) SetNillableParentID(id *xid.ID) *AssetUpdateOne {
if id != nil {
- auo = auo.SetParentID(*id)
+ auo.SetParentID(*id)
}
return auo
}📝 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.
| func (auo *AssetUpdateOne) SetNillableParentID(id *xid.ID) *AssetUpdateOne { | |
| if id != nil { | |
| auo = auo.SetParentID(*id) | |
| } | |
| return auo | |
| } | |
| func (auo *AssetUpdateOne) SetNillableParentID(id *xid.ID) *AssetUpdateOne { | |
| if id != nil { | |
| auo.SetParentID(*id) | |
| } | |
| return auo | |
| } |
| // SetPrimaryAssetID sets the "primary_asset_id" field. | ||
| func (nu *NodeUpdate) SetPrimaryAssetID(x xid.ID) *NodeUpdate { | ||
| nu.mutation.SetPrimaryAssetID(x) | ||
| return nu | ||
| } | ||
|
|
||
| // SetNillablePrimaryAssetID sets the "primary_asset_id" field if the given value is not nil. | ||
| func (nu *NodeUpdate) SetNillablePrimaryAssetID(x *xid.ID) *NodeUpdate { | ||
| if x != nil { | ||
| nu.SetPrimaryAssetID(*x) | ||
| } | ||
| return nu | ||
| } | ||
|
|
||
| // ClearPrimaryAssetID clears the value of the "primary_asset_id" field. | ||
| func (nu *NodeUpdate) ClearPrimaryAssetID() *NodeUpdate { | ||
| nu.mutation.ClearPrimaryAssetID() | ||
| return nu | ||
| } |
There was a problem hiding this comment.
Add Validation for "primary_asset_id" in NodeUpdate
Consider adding validation to ensure that the xid.ID provided to SetPrimaryAssetID corresponds to an existing Asset entity. This will help prevent runtime errors due to invalid references and maintain data integrity.
| // SetPrimaryAssetID sets the "primary_asset_id" field. | ||
| func (nuo *NodeUpdateOne) SetPrimaryAssetID(x xid.ID) *NodeUpdateOne { | ||
| nuo.mutation.SetPrimaryAssetID(x) | ||
| return nuo | ||
| } | ||
|
|
||
| // SetNillablePrimaryAssetID sets the "primary_asset_id" field if the given value is not nil. | ||
| func (nuo *NodeUpdateOne) SetNillablePrimaryAssetID(x *xid.ID) *NodeUpdateOne { | ||
| if x != nil { | ||
| nuo.SetPrimaryAssetID(*x) | ||
| } | ||
| return nuo | ||
| } | ||
|
|
||
| // ClearPrimaryAssetID clears the value of the "primary_asset_id" field. | ||
| func (nuo *NodeUpdateOne) ClearPrimaryAssetID() *NodeUpdateOne { | ||
| nuo.mutation.ClearPrimaryAssetID() | ||
| return nuo | ||
| } | ||
|
|
There was a problem hiding this comment.
Add Validation for "primary_asset_id" in NodeUpdateOne
Similar to the NodeUpdate struct, please add validation in the NodeUpdateOne methods to ensure the xid.ID provided is valid and exists. This ensures consistency and prevents potential runtime errors caused by invalid Asset references.
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (29)
web/src/lib/library/metadata.ts (2)
5-9: LGTM: CoverImage schema and type are well-defined.The CoverImageSchema correctly defines the structure for a cover image position with 'top' and 'left' properties. The use of
z.inferfor the CoverImage type ensures consistency between the schema and the type.Consider adding constraints to the number fields (e.g.,
z.number().min(0)) to ensure non-negative values for 'top' and 'left' if that aligns with your requirements.
16-25: LGTM: parseNodeMetadata function is well-implemented, with a suggestion for improvement.The function correctly uses
safeParsefor validation and handles both defined and undefined input. Returning an empty object as a fallback is a reasonable approach.Consider adding error logging when parsing fails. This could help with debugging in case of unexpected metadata formats:
export function parseNodeMetadata( metadata: Metadata | undefined, ): NodeMetadata { const parsed = NodeMetadataSchema.safeParse(metadata); if (parsed.success) { return parsed.data; } console.error('Failed to parse node metadata:', parsed.error); return {}; }web/src/api/openapi-schema/asset.ts (1)
17-17: LGTM: Addition ofparentproperty enhances Asset structureThe addition of the optional
parentproperty of typeAssetto theAssetinterface is a good enhancement. It allows for the representation of hierarchical relationships between assets, which aligns with the PR objectives, particularly the consideration of adding a recursive backreference to Assets for change versioning.However, please consider the following:
- Ensure that the implementation handles potential circular references to avoid infinite loops.
- Update any existing asset management and querying logic to account for this new hierarchical structure.
- Consider adding documentation or comments explaining the purpose and usage of this new property.
web/src/api/openapi-schema/parentAssetIDQueryParameter.ts (1)
5-7: Consider the implications of the current versioning strategy.The comment about the API versioning strategy is noteworthy:
The Storyden API does not adhere to semantic versioning but instead applies a rolling strategy with deprecations and minimal breaking changes. This has been done mainly for a simpler development process and it may be changed to a more fixed versioning strategy in the future. Ultimately, the primary way Storyden tracks versions is dates, there are no set release tags currently.While this approach may simplify the development process, it could potentially lead to challenges for API consumers. Consider the following points:
- Predictability: Without semantic versioning, it may be harder for consumers to anticipate the impact of updates.
- Dependency management: Rolling releases can complicate dependency management in client projects.
- Long-term support: The lack of fixed version tags might make it difficult to support older versions of the API.
It might be beneficial to revisit this strategy in the future and consider adopting a more structured versioning approach, such as semantic versioning, to provide clearer expectations for API consumers.
internal/ent/schema/asset.go (3)
31-31: LGTM! Consider adding a comment for clarity.The addition of the
parent_asset_idfield is well-implemented, allowing for a hierarchical structure of assets. The use ofOptional()andNillable()is appropriate for this relationship.Consider adding a brief comment explaining the purpose of this field for better code documentation:
+ // parent_asset_id represents the ID of the parent asset in a hierarchical structure field.String("parent_asset_id").GoType(xid.ID{}).Optional().Nillable(),
58-62: LGTM! Consider renaming the edge for clarity.The new edge correctly establishes a parent-child relationship between assets, complementing the
parent_asset_idfield. The use ofUnique()ensures a proper tree-like structure.For improved clarity, consider renaming the edge from "assets" to "child_assets" to make it more explicit that it refers to the children of the current asset:
- edge.To("assets", Asset.Type). + edge.To("child_assets", Asset.Type). From("parent"). Unique(). Field("parent_asset_id"),This change would make the relationship more immediately clear to other developers working with this schema.
31-31: Consider additional validations for the hierarchical structure.The introduction of a parent-child relationship for assets is a significant change that might affect various parts of the application. While the implementation looks solid, there are a few additional considerations:
- Preventing circular references: It might be beneficial to add a validation to ensure that an asset cannot be its own ancestor.
- Depth limitations: Depending on the use case, you might want to consider limiting the depth of the asset hierarchy.
- Cascading operations: Consider how operations like delete should behave for parent assets with children.
Would you like assistance in implementing any of these additional validations or in analyzing the potential impact on existing queries and operations?
Also applies to: 58-62
app/resources/asset/asset.go (3)
22-27: LGTM! Consider adding documentation.The new
AddVersionmethod is a good addition for implementing asset versioning. The method signature is consistent with other methods in the interface.Consider adding a brief comment explaining the purpose of this method and the significance of the
parentparameter.
46-46: LGTM! Consider adding documentation.The new
Parentfield of typeopt.Optional[Asset]is a good addition for implementing asset versioning. The use ofOptionalallows for nullable parent references, which is appropriate for this use case.Consider adding a brief comment explaining the purpose of this field and its role in asset versioning.
50-51: LGTM! Consider a minor optimization.The changes to
FromModelcorrectly implement the newParentfield, usingopt.NewPtrMapto handle the optional nature of the parent asset.Consider moving the
parentinitialization closer to where it's used in the struct initialization for better readability. Also, you might want to use a named function instead of a lambda for better clarity:func assetFromModel(a ent.Asset) Asset { return *FromModel(&a) } func FromModel(a *ent.Asset) *Asset { return &Asset{ ID: AssetID(a.ID), Name: Filename{ id: opt.New(a.ID), name: a.Filename, hasID: true, }, Size: a.Size, Metadata: a.Metadata, Parent: opt.NewPtrMap(a.Edges.Parent, assetFromModel), } }Also applies to: 61-61
web/src/api/openapi-schema/assetUploadParams.ts (1)
34-42: LGTM: New propertyparent_asset_idis well-defined and documented.The new
parent_asset_idproperty is correctly implemented as an optional field, maintaining backward compatibility. Its purpose and usage are clearly explained in the comment, which is great for developers using this type.Consider adding a brief example in the comment to illustrate how this property might be used in practice. This could further enhance the clarity for developers. For instance:
/** * ...existing comment... * * Example: * { * filename: "updated_image.jpg", * parent_asset_id: "asset_123" * } */web/src/components/ui/file-upload.tsx (3)
15-22: Approval: New RootProvider enhances file upload functionalityThe addition of the
RootProvidercomponent with its comprehensive type signature improves the overall structure and type safety of the file upload functionality. This aligns well with the PR's objective of enhancing library pages.For improved readability, consider using a type alias for the complex
Assigntype:type RootProviderPropsType = Assign< Assign<HTMLStyledProps<"div">, FileUpload.RootProviderBaseProps>, FileUploadVariantProps >; export const RootProvider = withProvider<HTMLDivElement, RootProviderPropsType>( FileUpload.RootProvider, "root" );This suggestion is optional and aims to enhance code maintainability.
24-31: Approval: Consistent update to Root componentThe update to the
Rootcomponent maintains consistency with the newly addedRootProviderand enhances type safety. The use ofwithProviderappropriately indicates its role in providing context to child components.For consistency with the previous suggestion and improved readability, consider using a type alias here as well:
type RootPropsType = Assign< Assign<HTMLStyledProps<"div">, FileUpload.RootBaseProps>, FileUploadVariantProps >; export const Root = withProvider<HTMLDivElement, RootPropsType>( FileUpload.Root, "root" );This suggestion is optional and aims to maintain consistency across the file.
83-86: Approval: Clear exports with minor suggestionThe export of
FileUploadContextandFileUploadHiddenInputfrom the external library, with renaming, is appropriate. This approach allows for consistent naming within your application while utilizing the functionality from the external library.For improved clarity, consider adding a comment explaining why these components are exported directly:
// Export these components directly as they are used without additional styling or context wrapping export { FileUploadContext as Context, FileUploadHiddenInput as HiddenInput, } from "@ark-ui/react/file-upload";This suggestion is optional and aims to enhance code readability and maintainability.
app/transports/http/bindings/assets.go (1)
51-52: LGTM! Consider adding a comment for clarity.The addition of the
parentIDvariable aligns well with the PR objective of introducing a new asset relation. The use ofopt.NewPtrMapcorrectly handles the optional nature of the parent asset ID.Consider adding a brief comment explaining the purpose of this new variable:
// Extract the optional parent asset ID from the request parameters parentID := opt.NewPtrMap(request.Params.ParentAssetId, deserialiseAssetID)web/src/components/asset/AssetUploadAction.tsx (1)
24-125: LGTM: Well-implemented AssetUploadAction componentThe component is well-structured with clear separation of concerns. It handles both file uploads and rejections effectively, and the UI dynamically changes based on the
operationprop. The use of thehandlefunction for error handling is a good practice.One minor suggestion:
Consider enhancing the error logging in the
handleFilefunction. Instead of just logging "no file was provided", it might be helpful to include more context, such as the length of thefilesarray. This could help in debugging potential issues with the file selection process.- console.error("handleFile: no file was provided", files); + console.error(`handleFile: no file was provided. Files array length: ${files.length}`, files);🧰 Tools
🪛 Biome
[error] 68-68: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
web/src/screens/library/LibraryPageScreen/LibraryPageScreen.tsx (5)
3-4: Improved component structure and image handling capabilities.The new imports and the separation of
LibraryPageScreenandLibraryPagecomponents enhance code organization and align well with the PR objectives. The addition of image-related imports supports the new cover image functionality.Consider grouping related imports together for better readability. For example, you could group all UI component imports and all API-related imports.
Also applies to: 11-11, 13-13, 17-17, 20-22, 27-32, 34-43, 45-45
Line range hint
46-62: Well-structured component with clear separation of concerns.The
LibraryPagecomponent effectively uses theuseLibraryPageScreenhook to manage its state and handlers. The addition of image-related props (e.g.,cropperRef,primaryAssetURL) aligns well with the PR objectives.Consider using destructuring for the
propsparameter to make it clear which props are expected:export function LibraryPage({ node }: Props) { // ... rest of the component }This change would improve code readability and self-documentation.
86-104: Improved UI structure for edit actions.The conditional rendering of edit actions using
HStackimproves the user interface and simplifies the code structure. This change enhances the user experience by showing relevant controls based on the editing state.For consistency, consider using a ternary operator instead of an if-else structure for the conditional rendering:
{isAllowedToEdit && ( <HStack> {editing ? ( <> <CancelAction type="button" onClick={handleEditMode}> Cancel </CancelAction> <SaveAction type="submit">Save</SaveAction> </> ) : ( <EditAction onClick={handleEditMode}>Edit</EditAction> )} <LibraryPageMenu node={node} onVisibilityChange={handleVisibilityChange} onDelete={handleDelete} /> </HStack> )}This would make the code more concise and easier to read.
122-174: Effective implementation of cover image functionality.The use of
FixedCropperfor editing andImagefor display effectively addresses the challenge of handling dynamic width while maintaining a fixed height for the cover image. This implementation aligns well with the PR objectives.Consider the following improvements:
- Extract the
FixedCropperandImagecomponents into separate, reusable components to improve code organization and reusability.- Use a constant for the height value (currently "64") to improve maintainability.
- Consider adding a fallback or placeholder for when
primaryAssetURLis not available in non-editing mode.Example of extracting the
FixedCroppercomponent:const CoverImageCropper = ({ url, cropperRef, initialCoordinates }) => ( <FixedCropper ref={cropperRef} className={css({ maxWidth: "full", maxHeight: "64", borderRadius: "lg", backgroundColor: "bg.default", })} // ... other props src={url} /> ); // Usage in the main component {editing && primaryAssetEditingURL ? ( <Box width="full" height="64"> <CoverImageCropper url={primaryAssetEditingURL} cropperRef={cropperRef} initialCoordinates={initialCoverCoordinates} /> </Box> ) : ( // ... Image component )}This refactoring would improve code readability and maintainability.
Line range hint
175-238: Consistent implementation of existing functionality.The structure for title, content, and link inputs, as well as the rendering of child pages, remains consistent and works well with the new changes.
Consider adding a comment or documentation for the
NodeCardRowscomponent to explain its purpose and expected props. This would improve code maintainability and make it easier for other developers to understand the component's role in the library page structure.internal/ent/migrate/schema.go (1)
123-123: Consider the broader impact of schema changesThe additions to both
AssetsColumnsandNodesColumns, along with their corresponding foreign key relationships, enhance the database schema in a consistent and logical manner. These changes support the PR objectives and add valuable functionality to the system.However, please consider the following points:
Database Migration: Ensure that a proper migration script is created to add these new columns and relationships to the existing database.
Application Logic: Update the application logic to utilize these new relationships, particularly for managing asset hierarchies and setting/retrieving primary assets for nodes.
Performance: Monitor query performance, especially for operations involving these new relationships, to ensure they don't introduce any significant performance overhead.
Documentation: Update any relevant documentation to reflect these new relationships and their intended use in the system.
Also applies to: 137-142, 550-550, 576-581
internal/ent/mutation.go (4)
4131-4178: New methods for parent_asset_id field handlingThe new methods for handling the "parent_asset_id" field in AssetMutation are well-implemented and consistent with the existing codebase. They provide comprehensive functionality for setting, getting, clearing, and resetting the parent asset ID.
One minor suggestion:
Consider adding a comment to explain the purpose of the parent-child relationship for assets. This would help developers understand the use case for this new functionality.
4382-4420: New methods for parent edge handling in AssetMutationThe new methods for handling the "parent" edge in AssetMutation are well-implemented and consistent with the existing edge handling patterns in the codebase. They provide comprehensive functionality for setting, getting, clearing, and resetting the parent edge.
One suggestion for improvement:
Consider adding a comment to explain the difference between
ParentCleared()andParentAssetIDCleared()methods, as their purposes might not be immediately clear to other developers.
15210-15258: New methods for primary_asset_id field handling in NodeMutationThe new methods for handling the "primary_asset_id" field in NodeMutation are well-implemented and consistent with the existing codebase. They provide comprehensive functionality for setting, getting, clearing, and resetting the primary asset ID.
One suggestion for improvement:
Consider adding a comment to explain the purpose and usage of the primary asset in the context of a Node. This would help developers understand the significance of this new field.
15527-15566: New methods for primary_image edge handling in NodeMutationThe new methods for handling the "primary_image" edge in NodeMutation are well-implemented and consistent with the existing edge handling patterns in the codebase. They provide comprehensive functionality for setting, getting, clearing, and resetting the primary image edge.
One suggestion for improvement:
Consider adding a comment to explain the difference between
PrimaryImageCleared()andPrimaryAssetIDCleared()methods, as their purposes might not be immediately clear to other developers.app/services/asset/asset_upload/uploader.go (1)
61-67: Consider adding unit tests for the new 'ParentID' functionalityAdding tests to cover scenarios where
ParentIDis provided or omitted will ensure that both code paths in the upload process function correctly.web/src/screens/library/LibraryPageScreen/useLibraryPageScreen.ts (2)
177-177: Address the TODO regarding asset deletionThere's a TODO comment indicating uncertainty about deleting the original asset. To manage storage efficiently and maintain data consistency, consider implementing logic to delete or archive the original asset if it's no longer needed.
Would you like assistance in implementing the asset deletion logic or opening a GitHub issue to track this task?
197-199: Address the TODO about filename and ID separationThe TODO comment suggests splitting the filename from the ID on the API side to use the original name. This could enhance clarity and usability when managing assets.
Do you need help implementing this feature or should we create a GitHub issue to track this improvement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (6)
api/config.yamlis excluded by!**/*.yamlapi/openapi.yamlis excluded by!**/*.yamlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumweb/package.jsonis excluded by!**/*.jsonweb/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (54)
- app/resources/asset/asset.go (3 hunks)
- app/resources/asset/db.go (1 hunks)
- app/resources/library/mapping.go (1 hunks)
- app/resources/library/node.go (1 hunks)
- app/resources/library/node_querier/node_querier.go (1 hunks)
- app/resources/library/node_search/search.go (1 hunks)
- app/resources/library/node_writer/node_writer.go (1 hunks)
- app/services/asset/asset_upload/uploader.go (2 hunks)
- app/services/library/node_mutate/node.go (3 hunks)
- app/transports/http/bindings/assets.go (2 hunks)
- app/transports/http/bindings/nodes.go (5 hunks)
- app/transports/http/openapi/server_gen.go (13 hunks)
- internal/deletable/deletable.go (1 hunks)
- internal/ent/asset.go (7 hunks)
- internal/ent/asset/asset.go (7 hunks)
- internal/ent/asset/where.go (3 hunks)
- internal/ent/asset_create.go (6 hunks)
- internal/ent/asset_query.go (8 hunks)
- internal/ent/asset_update.go (8 hunks)
- internal/ent/client.go (2 hunks)
- internal/ent/er.html (4 hunks)
- internal/ent/migrate/schema.go (6 hunks)
- internal/ent/mutation.go (41 hunks)
- internal/ent/node.go (14 hunks)
- internal/ent/node/node.go (7 hunks)
- internal/ent/node/where.go (3 hunks)
- internal/ent/node_create.go (6 hunks)
- internal/ent/node_query.go (8 hunks)
- internal/ent/node_update.go (8 hunks)
- internal/ent/schema/asset.go (2 hunks)
- internal/ent/schema/node.go (2 hunks)
- web/panda.config.ts (2 hunks)
- web/src/api/openapi-schema/asset.ts (1 hunks)
- web/src/api/openapi-schema/assetUploadParams.ts (2 hunks)
- web/src/api/openapi-schema/index.ts (1 hunks)
- web/src/api/openapi-schema/nodeCommonProps.ts (2 hunks)
- web/src/api/openapi-schema/nodeInitialProps.ts (2 hunks)
- web/src/api/openapi-schema/nodeMutableProps.ts (2 hunks)
- web/src/api/openapi-schema/nullableIdentifier.ts (1 hunks)
- web/src/api/openapi-schema/parentAssetIDQueryParameter.ts (1 hunks)
- web/src/components/asset/AssetUploadAction.tsx (1 hunks)
- web/src/components/library/LibraryPageCoverImageControl/LibraryPageCoverImageControl.tsx (1 hunks)
- web/src/components/library/LibraryPageCoverImageControl/useLibraryPageCoverImageControl.ts (1 hunks)
- web/src/components/library/LibraryPageImportFromURL/LibraryPageImportFromURL.tsx (1 hunks)
- web/src/components/ui/file-upload.tsx (1 hunks)
- web/src/lib/library/library.ts (7 hunks)
- web/src/lib/library/metadata.ts (1 hunks)
- web/src/recipes/file-upload.ts (1 hunks)
- web/src/screens/library/LibraryPageScreen/LibraryPageScreen.tsx (3 hunks)
- web/src/screens/library/LibraryPageScreen/useLibraryPageScreen.ts (7 hunks)
- web/src/screens/media/MediaEditModal.tsx (1 hunks)
- web/src/screens/media/MediaEditScreen.tsx (1 hunks)
- web/styled-system/recipes/file-upload.d.ts (1 hunks)
- web/styled-system/recipes/file-upload.mjs (1 hunks)
🔥 Files not summarized due to errors (2)
- app/transports/http/openapi/server_gen.go: Error: Server error: no LLM provider could handle the message
- internal/ent/mutation.go: Error: Server error: no LLM provider could handle the message
✅ Files skipped from review due to trivial changes (1)
- web/src/components/library/LibraryPageImportFromURL/LibraryPageImportFromURL.tsx
🚧 Files skipped from review as they are similar to previous changes (19)
- app/resources/library/mapping.go
- app/resources/library/node.go
- app/resources/library/node_querier/node_querier.go
- app/resources/library/node_search/search.go
- app/services/library/node_mutate/node.go
- app/transports/http/bindings/nodes.go
- internal/ent/node_create.go
- internal/ent/schema/node.go
- web/panda.config.ts
- web/src/api/openapi-schema/nodeCommonProps.ts
- web/src/api/openapi-schema/nodeInitialProps.ts
- web/src/api/openapi-schema/nodeMutableProps.ts
- web/src/components/library/LibraryPageCoverImageControl/LibraryPageCoverImageControl.tsx
- web/src/components/library/LibraryPageCoverImageControl/useLibraryPageCoverImageControl.ts
- web/src/recipes/file-upload.ts
- web/src/screens/media/MediaEditModal.tsx
- web/src/screens/media/MediaEditScreen.tsx
- web/styled-system/recipes/file-upload.d.ts
- web/styled-system/recipes/file-upload.mjs
🧰 Additional context used
🪛 Biome
web/src/components/asset/AssetUploadAction.tsx
[error] 68-68: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
🔇 Additional comments (168)
web/src/lib/library/metadata.ts (2)
1-3: LGTM: Imports are appropriate for the file's functionality.The imports for Zod and the Metadata type are correctly included and necessary for the schema definitions and parsing function.
11-14: LGTM: NodeMetadata schema and type are correctly implemented.The NodeMetadataSchema is well-structured, including an optional coverImage property that uses the previously defined CoverImageSchema. The use of
z.inferfor the NodeMetadata type ensures consistency with the schema.web/src/api/openapi-schema/nullableIdentifier.ts (2)
1-9: LGTM: Generated file header is informative and clear.The header provides valuable information about the file's generated nature and the API's versioning strategy. This is good practice for maintaining clarity in the codebase.
11-15: LGTM: Well-defined and documented type for nullable identifiers.The
NullableIdentifiertype is correctly defined asstring | null, aligning with the PR objectives. The JSDoc comment provides clear documentation, including the@nullabletag, which enhances code clarity and IDE support.web/src/api/openapi-schema/parentAssetIDQueryParameter.ts (1)
1-19: LGTM! Well-documented type definition.The implementation of
ParentAssetIDQueryParameteris clear, concise, and well-documented. The detailed comment explaining its purpose and use case is particularly helpful for developers who will be working with this type.web/src/api/openapi-schema/assetUploadParams.ts (1)
13-13: LGTM: New import statement is correctly added.The import for
ParentAssetIDQueryParameteris properly formatted and consistent with the existing import style. It's correctly used in the new property ofAssetUploadParams.web/src/components/ui/file-upload.tsx (4)
1-13: Improved type safety and modularity in importsThe updated import statements and new utility imports enhance the type safety and modularity of the component. The introduction of
Assignfrom "@ark-ui/react" andcreateStyleContextfrom a custom utility file suggests a more robust approach to combining types and managing styles.
33-46: Approval: Consistent and type-safe updates to Dropzone, ItemDeleteTrigger, and ItemGroupThe updates to these components demonstrate a consistent approach to enhancing type safety and context usage. The use of
withContextwith specific HTML element types ensures that each component correctly consumes the context provided byRootorRootProvider. TheAssigntype effectively combines HTML props with the base props from FileUpload, maintaining a high level of type safety.This consistent structure across components will improve maintainability and reduce the likelihood of type-related errors in the future.
63-81: Approval: Consistent updates to Item, ItemSizeText, Label, and TriggerThe updates to these components maintain the consistent approach seen throughout the file. Each component uses
withContextwith the appropriate HTML element type, and the type signatures effectively combine HTML props with the base props from FileUpload using theAssigntype.This consistency in structure and type safety across all components in the file is commendable. It will greatly improve the maintainability of the code and reduce the potential for type-related errors.
48-61:⚠️ Potential issueApproval for ItemName and ItemPreviewImage, concern about ItemPreview
The updates to
ItemNameandItemPreviewImageare consistent with the previous components, maintaining type safety and context usage.However, the
ItemPreviewcomponent still has an inconsistency between its element type (HTMLImageElement) and its props type (HTMLStyledProps<"div">). This issue was flagged in a previous review but hasn't been addressed.To resolve this, please update the
ItemPreviewcomponent as follows:export const ItemPreview = withContext< HTMLDivElement, Assign<HTMLStyledProps<"div">, FileUpload.ItemPreviewBaseProps> >(FileUpload.ItemPreview, "itemPreview");This change ensures consistency between the element type and the props type.
app/transports/http/bindings/assets.go (1)
59-59: LGTM! ParentID field added successfully.The addition of the
ParentIDfield to theasset_upload.Optionsstruct is consistent with the earlier changes and supports the PR objective of associating assets with a parent.web/src/components/asset/AssetUploadAction.tsx (3)
1-15: LGTM: Well-organized importsThe imports are well-structured, separating external library imports from internal project imports. All imported modules appear to be relevant to the component's functionality.
16-22: LGTM: Well-structured type definitionsThe type definitions for
AssetUploadActionPropsandPropsare clear and provide a good structure for the component's properties. The use of a union type for theoperationprop enhances type safety, and extendingAssetUploadActionPropsinPropsallows for flexible prop passing.
127-147: LGTM: Robust MIME type handlingThe
getMIMEsutility function is well-implemented and thoroughly handles all possible input types for theacceptparameter. The comment explaining the reason for its complexity is helpful for future maintainers. The function name accurately describes its purpose, enhancing code readability.app/resources/library/node_writer/node_writer.go (3)
76-80: LGTM: WithPrimaryImage function implementationThe
WithPrimaryImagefunction is well-implemented and aligns with the PR objectives. It correctly uses theOptionpattern and sets the primary asset ID as expected.
82-86: LGTM: WithPrimaryImageRemoved function implementationThe
WithPrimaryImageRemovedfunction is well-implemented and addresses the suggestion from the past review comment. It correctly uses theOptionpattern and clears the primary asset ID as expected.
76-86: Great addition of primary image management functionsThe new
WithPrimaryImageandWithPrimaryImageRemovedfunctions are excellent additions to thenode_writerpackage. They provide a clean and consistent way to manage the primary image for nodes, aligning perfectly with the PR objectives of adding a primary asset cover image to library pages.These functions:
- Follow the existing
Optionpattern, maintaining code consistency.- Offer both setting and removing capabilities for the primary image.
- Address the suggestion from the past review comment about image removal.
These additions enhance the flexibility of node management and improve the overall functionality of the library pages.
internal/ent/er.html (4)
43-43: LGTM: Asset parent-child relationship addedThe addition of
parent_asset_idto theAssetentity is a good implementation. This change enables the creation of relationships between assets, which aligns with the PR objective of handling original and cropped versions of images for library page cover images.
166-166: LGTM: Primary asset reference added to Node entityThe addition of
primary_asset_idto theNodeentity is an excellent implementation that directly fulfills the main objective of the PR. This change allows nodes (likely representing library pages) to have a primary image, which can serve as the cover image for library pages.
244-244: LGTM: New relationships added to support cover imagesThe new relationships added to the ERD are well-implemented and crucial for the cover image feature:
Asset |o--o{ Asset : "assets/parent": This relationship enables the parent-child structure for assets, supporting the handling of original and cropped versions of images.
Node }o--o| Asset : "primary_image": This relationship directly implements the cover image feature for library pages by linking a Node (library page) to its primary Asset (cover image).Both relationships use correct cardinality notation and are consistent with the new fields added to the entities.
Also applies to: 264-264
43-43: Summary: ERD updates successfully implement cover image featureThe changes made to the entity-relationship diagram effectively implement the cover image feature for library pages:
- The
Assetentity now has aparent_asset_id, allowing for parent-child relationships between assets.- The
Nodeentity has a newprimary_asset_id, enabling the association of a primary image with each node.- New relationships have been added to support these features:
AssettoAssetfor parent-child asset relationshipsNodetoAssetfor linking primary images to nodesThese modifications provide a solid foundation for managing cover images in library pages, supporting both original and cropped versions of images. The changes are consistent with the PR objectives and well-implemented within the existing data model.
Also applies to: 166-166, 244-244, 264-264
web/src/api/openapi-schema/index.ts (1)
252-252: LGTM! New exports added successfully.The new exports
nullableIdentifierandparentAssetIDQueryParameterhave been added correctly. These additions align with the PR objectives of adding a primary asset cover image to library pages.As this file is auto-generated, please ensure that these changes are properly reflected in the OpenAPI specification. You can verify this by running the following command:
If the types are not found, please update the OpenAPI specification accordingly.
Also applies to: 258-258
internal/ent/node/node.go (5)
37-38: LGTM: New constants for primary asset and image.The new constants for
FieldPrimaryAssetID,EdgePrimaryImage,PrimaryImageTable,PrimaryImageInverseTable, andPrimaryImageColumnare well-defined and consistent with the existing naming conventions. They provide the necessary information for database operations and relationships, aligning with the PR objective of adding a primary asset cover image to library pages.Also applies to: 51-52, 82-88
137-137: LGTM: FieldPrimaryAssetID added to Columns slice.The addition of
FieldPrimaryAssetIDto theColumnsslice is correct and consistent with the other fields. This ensures that the new field is properly recognized as part of the SQL columns for node fields.
262-265: LGTM: New ByPrimaryAssetID function added.The new
ByPrimaryAssetIDfunction is well-implemented and consistent with other ordering functions in the file. It correctly uses thesql.OrderByFieldfunction and aligns with the newFieldPrimaryAssetIDconstant, providing a way to order results by the primary_asset_id field.
409-415: LGTM: New newPrimaryImageStep function added.The
newPrimaryImageStepfunction is well-implemented and consistent with other step functions in the file. It correctly uses the previously defined constants and sets up an appropriate Many-to-One (M2O) relationship for the primary image. This function properly supports theByPrimaryImageFieldordering function defined earlier.
305-310: LGTM: New ByPrimaryImageField function added, but verify step function.The new
ByPrimaryImageFieldfunction is well-implemented and consistent with other ordering functions for related entities. It correctly usessqlgraph.OrderByNeighborTermsfor ordering.However, please note that this function depends on
newPrimaryImageStep(), which should be defined later in the file.✅ Verification successful
Verified:
newPrimaryImageStep()function is defined.The
newPrimaryImageStep()function is correctly defined ininternal/ent/node/node.goat line 409, satisfying the dependency for theByPrimaryImageFieldfunction.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that newPrimaryImageStep() is defined in the file grep -n "func newPrimaryImageStep()" internal/ent/node/node.goLength of output: 112
internal/ent/migrate/schema.go (2)
123-123: LGTM: Asset hierarchy implementationThe addition of the
parent_asset_idcolumn to theAssetsColumnsand the corresponding foreign key relationship in theAssetsTableis well-implemented. This change allows for a hierarchical structure of assets, which can be useful for organizing related assets (e.g., original and cropped versions of an image).The
SetNullaction on delete is appropriate, as it ensures that deleting a parent asset doesn't cascade to its children, maintaining data integrity.Also applies to: 137-142
550-550: LGTM: Primary asset for nodes implementedThe addition of the
primary_asset_idcolumn to theNodesColumnsand the corresponding foreign key relationship in theNodesTableis well-implemented. This change directly supports the PR objective of adding a primary asset (cover image) to library pages.The
SetNullaction on delete is appropriate, as it ensures that deleting an asset doesn't affect the node itself, only removing the reference to the primary asset.Also applies to: 576-581
internal/ent/node_query.go (4)
36-36: LGTM: New field for primary image query.The addition of the
withPrimaryImagefield to theNodeQuerystruct aligns with the PR objective of introducing a primary image for nodes. This field will likely be used for eager loading of the primary image asset.
542-552: LGTM: New method for eager loading of primary image.The
WithPrimaryImagemethod is a well-implemented addition that allows for eager loading of the primary image when querying nodes. It follows the same pattern as other similar methods in the file and provides flexibility through optional function arguments.
750-755: LGTM: Proper handling of primary image loading.The addition to the
sqlAllmethod correctly implements the loading of the primary image. It follows the same pattern as other relationship loading in the method and includes a proper check to ensurewithPrimaryImageis set before attempting to load.
888-919: LGTM: Well-implementedloadPrimaryImagemethod.The
loadPrimaryImagemethod is a solid implementation for loading primary images for nodes. It follows the established pattern of other loading methods in the file, correctly handles potential nilPrimaryAssetIDvalues, and includes proper error handling and query construction.internal/ent/mutation.go (29)
3765-3769: New fields added for parent-child relationship and asset managementThese new fields enhance the AssetMutation struct by introducing support for parent-child relationships and asset management within assets. The naming convention and types are consistent with the existing code style.
4564-4564: Updated field count in Fields() methodThe number of fields in the
Fields()method has been correctly updated from 6 to 7, likely to account for the new "parent_asset_id" field. This change ensures that the method accurately reports the number of fields in the struct.
4583-4585: Added parent_asset_id to Fields() methodThe "parent_asset_id" field has been correctly added to the
Fields()method. This ensures that the field is included in the list of fields when it's set, maintaining consistency with how other fields are handled in this method.
4606-4607: Added parent_asset_id to field access methodThe "parent_asset_id" field has been correctly added to the switch statement in the field access method. This ensures that the field can be accessed through the generic field access method, maintaining consistency with how other fields are handled.
4629-4630: Added parent_asset_id to OldField methodThe "parent_asset_id" field has been correctly added to the OldField method. This ensures that the old value of the field can be retrieved, maintaining consistency with how other fields are handled in this method.
4682-4688: Added parent_asset_id to SetField methodThe "parent_asset_id" field has been correctly added to the SetField method. The implementation includes proper type checking and error handling, and it uses the SetParentAssetID method to set the value. This is consistent with how other fields are handled in this method.
4737-4739: Added parent_asset_id to ClearedFields methodThe "parent_asset_id" field has been correctly added to the ClearedFields method. This ensures that the field is included in the list of cleared fields when it's cleared, maintaining consistency with how other fields are handled in this method.
4757-4759: Added parent_asset_id to ClearField methodThe "parent_asset_id" field has been correctly added to the ClearField method. The implementation calls the ClearParentAssetID method to clear the field, and the error handling is consistent with other cases in this method.
4786-4795: Added parent_asset_id reset and updated edge countThe "parent_asset_id" field has been correctly added to the reset method, calling ResetParentAssetID to reset the field. The error handling is consistent with other cases in this method.
Additionally, the number of edges in the AddedEdges method has been updated from 6 to 7, likely to account for the new parent-child relationship.
4808-4813: Added parent and assets edges to AddedEdges methodThe "parent" and "assets" edges have been correctly added to the AddedEdges method. This ensures that these new relationships are included in the list of added edges when they're set, maintaining consistency with how other edges are handled in this method.
4846-4855: Added parent and assets edges to EdgeIDs methodThe "parent" and "assets" edges have been correctly added to the EdgeIDs method. The implementation for the "parent" edge is consistent with other single-entity edges, while the "assets" edge correctly handles multiple IDs. Both implementations follow the established patterns in the codebase.
4868-4868: Updated edge count in RemovedEdges methodThe number of edges in the
RemovedEdges()method has been correctly updated from 6 to 7. This change likely accounts for the new parent-child relationship or the assets edge, ensuring that the method accurately reports the number of possible edges that can be removed.
4878-4880: Added assets edge to RemovedEdges methodThe "assets" edge has been correctly added to the RemovedEdges method. This ensures that the edge is included in the list of removed edges when it's removed, maintaining consistency with how other edges are handled in this method.
4909-4914: Added assets edge to RemovedEdgeIDs methodThe "assets" edge has been correctly added to the RemovedEdgeIDs method. The implementation properly handles multiple IDs for the "assets" edge and follows the established patterns in the codebase for handling removed edge IDs.
4927-4927: Updated edge count in ClearedEdges methodThe number of edges in the
ClearedEdges()method has been correctly updated from 6 to 7. This change likely accounts for the new parent-child relationship or the assets edge, ensuring that the method accurately reports the number of possible edges that can be cleared.
4940-4945: Added parent and assets edges to ClearedEdges methodThe "parent" and "assets" edges have been correctly added to the ClearedEdges method. This ensures that these edges are included in the list of cleared edges when they're cleared, maintaining consistency with how other edges are handled in this method.
4964-4967: Added parent and assets edges to EdgeCleared methodThe "parent" and "assets" edges have been correctly added to the EdgeCleared method. The implementation returns the appropriate boolean values for the cleared state of these edges, maintaining consistency with how other edges are handled in this method.
4981-4983: Added parent edge to ClearEdge methodThe "parent" edge has been correctly added to the ClearEdge method. The implementation calls the ClearParent method to clear the edge, and the error handling is consistent with other cases in this method.
5004-5009: Added parent and assets edges to ResetEdge methodThe "parent" and "assets" edges have been correctly added to the ResetEdge method. The implementation calls the ResetParent and ResetAssets methods to reset the respective edges. The error handling is consistent with other cases in this method.
14709-14710: New fields added for primary image in NodeMutationThese new fields enhance the NodeMutation struct by introducing support for a primary image in nodes. The naming convention and types are consistent with the existing code style.
15844-15844: Updated field count in Fields() method for NodeMutationThe number of fields in the
Fields()method has been correctly updated from 12 to 13, likely to account for the new "primary_asset_id" field in the Node struct. This change ensures that the method accurately reports the number of fields in the struct.
15872-15874: Added primary_asset_id to Fields() method in NodeMutationThe "primary_asset_id" field has been correctly added to the
Fields()method in NodeMutation. This ensures that the field is included in the list of fields when it's set, maintaining consistency with how other fields are handled in this method.
15910-15911: Added primary_asset_id to field access method in NodeMutationThe "primary_asset_id" field has been correctly added to the switch statement in the field access method of NodeMutation. This ensures that the field can be accessed through the generic field access method, maintaining consistency with how other fields are handled.
15945-15946: Added primary_asset_id to OldField method in NodeMutationThe "primary_asset_id" field has been correctly added to the OldField method in NodeMutation. This ensures that the old value of the field can be retrieved, maintaining consistency with how other fields are handled in this method.
16025-16031: Added primary_asset_id to SetField method in NodeMutationThe "primary_asset_id" field has been correctly added to the SetField method in NodeMutation. The implementation includes proper type checking and error handling, and it uses the SetPrimaryAssetID method to set the value. This is consistent with how other fields are handled in this method.
16095-16097: Added primary_asset_id to ClearedFields method in NodeMutationThe "primary_asset_id" field has been correctly added to the ClearedFields method in NodeMutation. This ensures that the field is included in the list of cleared fields when it's cleared, maintaining consistency with how other fields are handled in this method.
16130-16132: Added primary_asset_id to ClearField method in NodeMutationThe "primary_asset_id" field has been correctly added to the ClearField method in NodeMutation. The implementation calls the ClearPrimaryAssetID method to clear the field, and the error handling is consistent with other cases in this method.
16174-16176: Added primary_asset_id reset to NodeMutationThe "primary_asset_id" field has been correctly added to the reset method in NodeMutation, calling ResetPrimaryAssetID to reset the field. The error handling is consistent with other cases in this method.
16192-16192: Updated edge count in AddedEdges method for NodeMutationThe number of edges in the
AddedEdges()method of NodeMutation has been correctly updated from 8 to 9. This change likely accounts for the new primary image edge, ensuring that the method accurately reports the number of possible edges that can be added.app/transports/http/openapi/server_gen.go (12)
Line range hint
23-27: LGTM: Addition of nullable package importThe addition of the "github.com/oapi-codegen/nullable" import is appropriate and aligns with the need to handle nullable fields in OpenAPI specifications.
460-463: LGTM: Addition of Parent field to Asset structThe introduction of the
Parent *Assetfield creates a self-referential structure for Assets, enabling a parent-child relationship. The use of a pointer and theomitemptyJSON tag are appropriate for this optional field.
1453-1456: LGTM: Addition of Parent and PrimaryImage fields to Node structThe introduction of
Parent *NodeandPrimaryImage *Assetfields aligns well with the PR objectives. ThePrimaryImagefield enables the addition of a primary image to library pages, while theParentfield allows for a hierarchical structure of nodes. The use of pointers andomitemptytags is appropriate for these optional fields.
1493-1496: Please clarify the purpose of this duplicate codeThis code segment is identical to the previous one (lines 1453-1456). Could you please clarify if this duplication is intentional? If it's defining the same fields for different structs or in different contexts, it might be beneficial to add a comment explaining the reason for the duplication.
1526-1528: LGTM: Addition of PrimaryImageAssetId fieldThe introduction of the
PrimaryImageAssetId *AssetIDfield aligns well with the PR objectives of adding a primary image to library pages. This field allows referencing the primary image asset by its ID. The use of a pointer and theomitemptyJSON tag is appropriate for this optional field.
1568-1570: LGTM: Addition of nullable PrimaryImageAssetId fieldThe introduction of the
PrimaryImageAssetId nullable.Nullable[NullableIdentifier]field aligns with the PR objectives of adding a primary image to library pages. The use ofnullable.Nullableallows for explicit representation of null values, which can be beneficial in certain scenarios. This approach differs from the pointer-based approach seen earlier but serves a similar purpose.
1690-1692: LGTM: Addition of NullableIdentifier typeThe introduction of the
NullableIdentifiertype alias forstringis a good practice. It provides a clear and specific type for nullable identifiers, which can improve code readability and type safety throughout the codebase. This change supports the earlier additions of nullable fields.
2672-2674: LGTM: Addition of ParentAssetIDQuery typeThe introduction of the
ParentAssetIDQuerytype alias forstringis a good practice. It provides a clear and specific type for parent asset ID queries, which can improve code readability and type safety. This change supports the new parent-child relationship functionality for assets.
3060-3066: LGTM: Addition of ParentAssetId field with excellent documentationThe introduction of the
ParentAssetId *ParentAssetIDQueryfield aligns well with the PR objectives of supporting versioning for assets. The field allows specifying a parent asset when uploading new versions of an existing asset. The use of a pointer andomitemptytags is appropriate for this optional field.The detailed comment accompanying this field is particularly commendable. It provides clear guidance on the field's purpose, usage, and implications, which will be very helpful for developers using this API.
6161-6176: LGTM: Correct implementation of ParentAssetId query parameter handlingThe added logic for handling the
ParentAssetIdquery parameter is consistent with the previous additions and follows the established pattern for parameter processing. The implementation correctly usesruntime.StyleParamWithLocationandurl.ParseQueryfunctions, and includes proper error handling. This change ensures that the newParentAssetIdfield can be properly processed in API requests.
18168-18174: LGTM: Correct binding of parent_asset_id query parameterThe added logic for binding the
parent_asset_idquery parameter to theParentAssetIdfield is consistent with the previous additions and follows the established pattern for parameter binding. The implementation correctly uses theruntime.BindQueryParameterfunction and includes proper error handling, returning an HTTP 400 error with a descriptive message in case of an invalid format. This change ensures that the newParentAssetIdfield can be properly populated from API requests.
27628-27944: Update to Swagger specification acknowledgedThis change updates the base64 encoded, gzipped, JSON marshaled Swagger object, which likely includes the new fields and endpoints related to the primary image and asset versioning features. While the encoded content can't be directly reviewed here, it's an expected change given the modifications made to the API.
If a detailed review of the API specification changes is required, please decode and examine the content manually or provide a decoded version for review.
internal/deletable/deletable.go (1)
13-16: Well-definedValue[T]struct for managing deletable fields.The
Value[T]struct is clearly defined and well-documented, providing a type-safe way to handle deletable fields in resources.app/services/asset/asset_upload/uploader.go (1)
52-52: Addition of 'ParentID' field enhances functionalityThe introduction of the
ParentIDfield to theOptionsstruct allows for optional asset versioning during uploads, aligning with the PR objectives.internal/ent/asset.go (11)
35-36: Addition of ParentAssetID field is appropriateThe
ParentAssetIDfield is correctly added to handle the parent asset relationship.
53-56: Correct addition of Parent and Assets edgesThe
ParentandAssetsedges are appropriately added to represent the parent-child relationships between assets.
61-61: Ensure consistency of loadedTypes indices with new edgesThe
loadedTypesarray size has been increased to 7 to include the new edges. Please verify that all methods usingloadedTypesare updated accordingly to prevent any indexing errors.
102-111: Correct implementation of ParentOrErr methodThe
ParentOrErrmethod is properly implemented to retrieve theParentedge, with appropriate error handling.
113-120: Correct implementation of AssetsOrErr methodThe
AssetsOrErrmethod correctly handles retrieval of theAssetsedge, ensuring proper error handling when the edge is not loaded.
125-127: Verify updated index in EventOrErr methodThe index
e.loadedTypes[6]in theEventOrErrmethod has been updated. Please verify that this index correctly corresponds to theEventedge after adding new edges to ensure proper functionality.
136-137: Appropriate handling of ParentAssetID in scanValuesThe
scanValuesmethod correctly handles theParentAssetIDfield by usingsql.NullScannerfor scanning nullable values.
207-213: Correct assignment of ParentAssetID in assignValuesThe
assignValuesmethod appropriately handles the assignment of the nullableParentAssetIDfield, ensuring that the value is correctly set when valid.
247-250: Correct implementation of QueryParent methodThe
QueryParentmethod is correctly implemented to allow querying of theparentedge of theAssetentity.
252-255: Correct implementation of QueryAssets methodThe
QueryAssetsmethod properly facilitates querying theassetsedge of theAssetentity.
302-306: Correct inclusion of ParentAssetID in String methodThe
Stringmethod properly includes theParentAssetIDwhen it is notnil, ensuring accurate string representation of theAssetentity.internal/ent/asset/asset.go (10)
30-31: Addition ofFieldParentAssetIDis appropriateThe definition of
FieldParentAssetIDaligns with existing field definitions and follows naming conventions.
40-43: Definition ofEdgeParentandEdgeAssetsconstantsThe added edge constants
EdgeParentandEdgeAssetscorrectly define the edge names for mutations and are consistent with existing patterns.
70-77: Self-referential edge table and column definitions are correctThe definitions for
ParentTable,ParentColumn,AssetsTable, andAssetsColumnappropriately establish self-referential relationships within theassetstable.
96-96: IncludingFieldParentAssetIDinColumnsAdding
FieldParentAssetIDto theColumnsslice ensures it will be included in SQL queries and operations, which is necessary for the new field.
167-170: Implementation ofByParentAssetIDfunctionThe
ByParentAssetIDfunction correctly enables ordering of query results by theparent_asset_idfield, enhancing query capabilities.
221-226: Addition ofByParentFieldfunction for flexible orderingThe
ByParentFieldfunction allows ordering assets based on a specified field of their parent, providing more flexibility in queries.
228-233: Definition ofByAssetsCountfunctionThe
ByAssetsCountfunction enables ordering assets by the count of their related assets, which can be useful for analytics and reporting.
235-240: Addition ofByAssetsfunction for advanced orderingThe
ByAssetsfunction allows ordering based on terms from related assets, offering more advanced query options.
283-289: Correct implementation ofnewParentStepfunctionThe
newParentStepfunction accurately defines the traversal step for the parent edge, facilitating queries involving parent assets.
290-296: Proper definition ofnewAssetsStepfunctionThe
newAssetsStepfunction correctly sets up the traversal step for the assets edge, enabling navigation to child assets in queries.internal/ent/node.go (8)
14-14: Approved: Added necessary import forassetpackageThe import statement
"github.com/Southclaws/storyden/internal/ent/asset"is required for the use of*Assetin the code.
65-66: Approved: AddedPrimaryImageedge toNodeEdgesThe
PrimaryImageedge is correctly added toNodeEdges, allowing the node to reference its primary image asset.
81-81: Approved: UpdatedloadedTypesarray sizeThe
loadedTypesarray size is updated to[10]boolto include the newPrimaryImageedge. This ensures proper tracking of loaded types during eager loading.
115-125: Approved: AddedPrimaryImageOrErrmethodThe
PrimaryImageOrErrmethod is added toNodeEdges, adhering to the established pattern for edge accessors. It allows safe retrieval of thePrimaryImageedge with appropriate error handling.
187-188: Approved: HandledPrimaryAssetIDinscanValuesThe
scanValuesmethod correctly includes a case fornode.FieldPrimaryAssetID, preparing asql.NullScannerto handle potential null values.
275-281: Approved: AssignedPrimaryAssetIDinassignValuesThe
assignValuesmethod properly handles the assignment ofPrimaryAssetID, checking the type and validity before assignment to accommodate null values.
330-333: Approved: AddedQueryPrimaryImagemethodThe
QueryPrimaryImagemethod is added to theNodestruct, enabling querying of thePrimaryImageedge, consistent with other query methods.
421-425: Approved: IncludedPrimaryAssetIDinStringmethodThe
Stringmethod now includes thePrimaryAssetIDwhen constructing the string representation of theNode, ensuring comprehensive output.internal/ent/asset/where.go (4)
84-88: Addition ofParentAssetIDPredicate FunctionThe function
ParentAssetIDcorrectly applies an equality check on theparent_asset_idfield, consistent with other predicate functions in the file. The implementation is correct and aligns with established patterns.
354-433: Correct Implementation of Predicate Functions forParentAssetIDThe suite of predicate functions for
ParentAssetID(e.g.,ParentAssetIDEQ,ParentAssetIDNEQ,ParentAssetIDIn, etc.) are correctly implemented following the established patterns in the codebase. The handling ofxid.IDtypes and conversion to strings where necessary is appropriate.
526-547: Addition of Edge PredicatesHasParentandHasParentWithThe functions
HasParentandHasParentWithare correctly added to handle the "parent" edge predicates. The implementation aligns with existing edge predicate patterns and will facilitate querying parent relationships effectively.
549-570: Addition of Edge PredicatesHasAssetsandHasAssetsWithThe functions
HasAssetsandHasAssetsWithare appropriately implemented for the "assets" edge predicates. This addition follows the standard pattern used in the file and extends the querying capabilities for related assets.internal/ent/asset_create.go (7)
84-88: LGTM!The new
SetParentAssetIDmethod correctly sets theparent_asset_idfield in theAssetCreatemutation.
90-96: LGTM!The new
SetNillableParentAssetIDmethod correctly sets theparent_asset_idfield if the provided value is not nil, allowing for optional parent asset relationships.
585-601: LGTM!The new
SetParentAssetID,UpdateParentAssetID, andClearParentAssetIDmethods in theAssetUpsertcorrectly handle setting, updating, and clearing theparent_asset_idfield during upsert operations.
738-757: LGTM!The new
SetParentAssetID,UpdateParentAssetID, andClearParentAssetIDmethods in theAssetUpsertOnecorrectly handle setting, updating, and clearing theparent_asset_idfield during upsert operations for a single asset.
1061-1080: LGTM!The new
SetParentAssetID,UpdateParentAssetID, andClearParentAssetIDmethods in theAssetUpsertBulkcorrectly handle setting, updating, and clearing theparent_asset_idfield during bulk upsert operations.
412-428: Verify the cardinality of the parent asset relationship.The code assumes a many-to-one relationship between assets and their parent, which aligns with the method names
SetParentAssetIDandSetNillableParentAssetID. However, it's crucial to verify that this matches the intended cardinality of the parent-child asset relationship in the schema design.Run the following script to confirm the schema definition:
429-444: Verify the cardinality of the child assets relationship.The code assumes a one-to-many relationship between assets and their child assets, which aligns with the method name
AddAssetIDs. However, it's important to verify that this matches the intended cardinality of the parent-child asset relationship in the schema design.Run the following script to confirm the schema definition:
internal/ent/asset_query.go (11)
36-37: Fields Added to Support Parent and Assets RelationsThe addition of
withParent *AssetQueryandwithAssets *AssetQueryin theAssetQuerystruct extends the querying capabilities to include parent assets and associated assets.
164-184: Implementation ofQueryParentMethodThe
QueryParentmethod allows querying the parent asset associated with a given asset. The implementation follows the pattern used in other edge query methods and correctly establishes the many-to-one relationship.
186-207: Implementation ofQueryAssetsMethodThe
QueryAssetsmethod enables querying child assets linked to an asset. It correctly sets up the one-to-many relationship and mirrors the structure of existing edge query methods.
426-427: Ensuring Cloned Queries Maintain New RelationshipsThe
Clonemethod now includeswithParent: aq.withParent.Clone()andwithAssets: aq.withAssets.Clone(). This ensures that when anAssetQueryis cloned, it retains the state of the newwithParentandwithAssetsrelationships.
480-490: Addition ofWithParentMethodThe
WithParentmethod enables eager loading of the parent asset when querying assets. This is consistent with the eager-loading pattern used elsewhere in the code.
491-501: Addition ofWithAssetsMethodThe
WithAssetsmethod allows for eager loading of child assets associated with an asset. The implementation aligns with existing conventions for edge eager loading.
591-597: Updating Loaded Types for New RelationshipsIn the
sqlAllmethod, theloadedTypesarray now includesaq.withParent != nilandaq.withAssets != nil. This ensures that the query loader properly handles the new relationships when executing queries.
649-661: Loading Parent and Assets in QueriesThe
loadParentandloadAssetsfunctions have been added to handle the loading of parent and child assets, respectively. They correctly manage foreign key relationships and populate the edges as intended.
884-915: Implementation ofloadParentFunctionThe
loadParentfunction effectively retrieves and assigns the parent asset to each asset in the query result. It handles cases where theParentAssetIDmay be null and includes error handling for unexpected foreign keys.
916-948: Implementation ofloadAssetsFunctionThe
loadAssetsfunction appropriately handles the loading of child assets. It constructs the correct query to fetch related assets and assigns them to the corresponding parent assets.
1012-1014: IncludingParentAssetIDin Node Columns When NecessaryIn the
querySpecmethod, the code now adds theParentAssetIDcolumn whenaq.withParentis not nil. This ensures that the parent asset ID is available when needed for queries involving the parent relationship.internal/ent/node/where.go (17)
104-108: Consistent addition ofPrimaryAssetIDpredicateThe
PrimaryAssetIDfunction and its associated comment are correctly added, maintaining consistency with other field predicates. This enhances querying capabilities based on theprimary_asset_idfield.
674-678: Correct implementation ofPrimaryAssetIDEQpredicateThe
PrimaryAssetIDEQfunction is properly implemented, following the standard pattern for equality predicates on theprimary_asset_idfield.
679-683: Accurate addition ofPrimaryAssetIDNEQpredicateThe
PrimaryAssetIDNEQfunction correctly applies the NEQ predicate on theprimary_asset_idfield, enabling not-equal queries.
684-688: Proper addition ofPrimaryAssetIDInpredicateThe
PrimaryAssetIDInfunction allows querying for nodes withprimary_asset_idin a given list, which is correctly implemented.
689-693: Correct addition ofPrimaryAssetIDNotInpredicateThe
PrimaryAssetIDNotInfunction is properly implemented to exclude specifiedprimary_asset_idvalues from queries.
694-698: Addition ofPrimaryAssetIDGTpredicateThe
PrimaryAssetIDGTfunction correctly applies the greater-than predicate on theprimary_asset_idfield.
699-703: Implementation ofPrimaryAssetIDGTEpredicateThe
PrimaryAssetIDGTEfunction is correctly added, enabling greater-than-or-equal queries.
704-708: Correct addition ofPrimaryAssetIDLTpredicateThe
PrimaryAssetIDLTfunction properly implements the less-than predicate on theprimary_asset_idfield.
709-713: Implementation ofPrimaryAssetIDLTEpredicateThe
PrimaryAssetIDLTEfunction is correctly added, allowing less-than-or-equal queries.
714-719: Ensure proper conversion inPrimaryAssetIDContainspredicateThe
PrimaryAssetIDContainsfunction correctly converts thexid.IDto a string for substring searches. The implementation is accurate.
720-725: Correct implementation ofPrimaryAssetIDHasPrefixpredicateThe
PrimaryAssetIDHasPrefixfunction properly handles prefix checks on theprimary_asset_idfield.
726-731: Addition ofPrimaryAssetIDHasSuffixpredicateThe
PrimaryAssetIDHasSuffixfunction is correctly implemented for suffix checks.
732-736: Correct use ofIsNilpredicate inPrimaryAssetIDIsNilThe
PrimaryAssetIDIsNilfunction appropriately handles null value checks for theprimary_asset_idfield.
737-741: Implementation ofPrimaryAssetIDNotNilpredicateThe
PrimaryAssetIDNotNilfunction is correctly added, allowing queries for non-nullprimary_asset_idvalues.
742-747: Proper addition ofPrimaryAssetIDEqualFoldpredicateThe
PrimaryAssetIDEqualFoldfunction correctly enables case-insensitive equality checks on theprimary_asset_idfield.
748-753: Correct implementation ofPrimaryAssetIDContainsFoldpredicateThe
PrimaryAssetIDContainsFoldfunction allows for case-insensitive substring searches, which is properly implemented.
933-954: Implementation ofHasPrimaryImageandHasPrimaryImageWithpredicatesThe addition of
HasPrimaryImageandHasPrimaryImageWithfunctions correctly enables edge predicates for theprimary_imagerelationship, enhancing query capabilities related to the primary image of nodes.internal/ent/asset_update.go (17)
111-117:SetNillableParentAssetIDmethod follows best practices.The method correctly sets the
parent_asset_idfield if the provided value is not nil, maintaining consistency with other nillable setter methods.
119-123:ClearParentAssetIDmethod implementation is appropriate.The method appropriately clears the value of the
parent_asset_idfield in the mutation.
187-193:SetNillableParentIDmethod is consistent with patterns.The method correctly sets the "parent" edge if the provided ID is not nil, aligning with established patterns for nillable edges.
195-198:SetParentmethod correctly assigns the parent asset.The method sets the "parent" edge using an
Assetentity, enhancing usability when the parent entity is already available.
200-213: Addition ofAddAssetIDsandAddAssetsmethods enhances functionality.These methods allow adding multiple child assets to the current asset, facilitating the creation of asset hierarchies.
304-308:ClearParentmethod correctly clears the parent edge.The method appropriately clears the "parent" edge from the mutation.
310-329:ClearAssets,RemoveAssetIDs, andRemoveAssetsmethods are properly implemented.These methods provide functionality to clear or remove child asset associations, offering flexibility in managing asset relationships.
804-823: Added methods inAssetUpdateOneforparent_asset_idfield are consistent.The methods
SetParentAssetID,SetNillableParentAssetID, andClearParentAssetIDmirror those inAssetUpdate, maintaining consistency across builders.
880-898:SetParentID,SetNillableParentID, andSetParentmethods are properly added toAssetUpdateOne.These methods extend the functionality to update the "parent" edge in single-entity updates, aligning with bulk update capabilities.
899-913: Inclusion ofAddAssetIDsandAddAssetsmethods enhancesAssetUpdateOne.Adding these methods allows for associating multiple child assets during a single-entity update, providing consistency with bulk update operations.
1003-1008:ClearParentmethod inAssetUpdateOnecorrectly clears the parent edge.The method is implemented appropriately, allowing the parent association to be removed in single-entity updates.
1009-1029: Child asset management methods inAssetUpdateOneare correctly implemented.Methods to clear and remove child assets (
ClearAssets,RemoveAssetIDs,RemoveAssets) provide necessary functionality for managing relationships.
105-109:⚠️ Potential issueConfirm modifications to generated code.
This file is generated by Ent (
// Code generated by ent, DO NOT EDIT.). Direct modifications may be overwritten when the code is regenerated. It's recommended to update the schema definitions and regenerate the code to include these changes.Would you like to update the Ent schema and regenerate the code to incorporate these changes?
181-185: Validate the existence of theparentedge in the schema.The
SetParentIDmethod sets the "parent" edge to theAssetentity by ID. Ensure that theparentedge is defined in theAssetschema.Run the following script to confirm the
parentedge definition:#!/bin/bash # Description: Verify the 'parent' edge in the Asset schema. # Test: Search for the 'parent' edge definition. rg --type go 'Edge.*"parent"' ./internal/ent/schema/asset.go
596-669: Ensure edge specifications insqlSavealign with schema definitions.The
sqlSavemethod includes logic for handling the "parent" and "assets" edges. Verify that the edge specifications match the relationships defined in the schema.Run the following script to compare edge definitions:
#!/bin/bash # Description: Compare edge definitions in 'sqlSave' with the schema. # Test: Extract edge definitions from the schema. rg --type go 'Edges\(\)' -A 15 ./internal/ent/schema/asset.go # Test: Extract edge specifications from 'asset_update.go'. rg --type go 'EdgeSpec{' -A 10 ./internal/ent/asset_update.go
1325-1398: Edge handling insqlSavemethod forAssetUpdateOnelooks appropriate.The method includes logic for updating the "parent" and "assets" edges. Ensure that this logic is consistent with the schema definitions and behaves as expected during single-entity updates.
Run the following script to verify edge handling:
#!/bin/bash # Description: Verify edge handling in 'sqlSave' for AssetUpdateOne. # Test: Compare edge specifications with schema definitions. rg --type go 'EdgeSpec{' -A 10 ./internal/ent/asset_update.go rg --type go 'Edges\(\)' -A 15 ./internal/ent/schema/asset.go
105-109: EnsureSetParentAssetIDaligns with schema definitions.The method
SetParentAssetIDsets theparent_asset_idfield. Please verify that this field is defined in the Ent schema for theAssetentity.Run the following script to check if
parent_asset_idis defined in the schema:internal/ent/node_update.go (4)
166-185: Methods forprimary_asset_idfield are correctly implementedThe methods
SetPrimaryAssetID,SetNillablePrimaryAssetID, andClearPrimaryAssetIDin theNodeUpdatestruct follow established patterns and are correctly implemented.
399-404: MethodClearPrimaryImageis correctly implementedThe
ClearPrimaryImagemethod correctly clears theprimary_imageedge in theNodeUpdatestruct.
1101-1120: Methods forprimary_asset_idfield inNodeUpdateOneare correctly implementedThe methods
SetPrimaryAssetID,SetNillablePrimaryAssetID, andClearPrimaryAssetIDin theNodeUpdateOnestruct are consistent with existing mutation methods and are correctly implemented.
1334-1339: MethodClearPrimaryImageinNodeUpdateOneis correctly implementedThe
ClearPrimaryImagemethod properly clears theprimary_imageedge in theNodeUpdateOnestruct.internal/ent/client.go (3)
1332-1346: Well-implementedQueryParentmethod inAssetClientThe
QueryParentmethod correctly constructs a query to retrieve the parent asset associated with a given asset. The method properly sets up the many-to-one relationship usingsqlgraph.NewStepwith the correct parameters.
1348-1362: Well-implementedQueryAssetsmethod inAssetClientThe
QueryAssetsmethod accurately constructs a query to fetch the child assets of a given asset, representing the one-to-many relationship. The use ofsqlgraph.NewStepand the specified edge parameters are appropriate and consistent with the codebase.
3571-3585: Well-implementedQueryPrimaryImagemethod inNodeClientThe
QueryPrimaryImagemethod correctly adds functionality to query the primary image associated with a node. It properly establishes the many-to-one relationship and constructs the query usingsqlgraph.NewStep, aligning with existing patterns in the codebase.
| func (d *database) AddVersion(ctx context.Context, | ||
| accountID xid.ID, | ||
| parent AssetID, | ||
| filename Filename, | ||
| size int, | ||
| ) (*Asset, error) { | ||
| asset, err := d.db.Asset. | ||
| Create(). | ||
| SetID(filename.GetID()). | ||
| SetParentAssetID(parent). | ||
| SetFilename(filename.name). | ||
| SetSize(size). | ||
| SetAccountID(xid.ID(accountID)). | ||
| Save(ctx) | ||
| if err != nil { | ||
| return nil, fault.Wrap(err, fctx.With(ctx), ftag.With(ftag.Internal)) | ||
| } | ||
|
|
||
| return FromModel(asset), nil | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
AddVersion method lacks validation for parent asset existence.
The AddVersion method in app/resources/asset/db.go does not currently verify if the parent asset exists before creating a new version. Implementing the following improvements is recommended:
-
Check Parent Asset Exists:
- Use the
GetByIDmethod to ensure the parent asset exists.
- Use the
-
Validate Parent Parameter:
- Ensure the
parentparameter is not empty or invalid.
- Ensure the
-
Handle Filename Conflicts:
- Prevent creating a version with a filename that already exists to avoid conflicts.
Implementing these validations will enhance the method's reliability and prevent potential issues related to invalid parent assets or duplicate filenames.
🔗 Analysis chain
Consider adding validation and error handling for the new AddVersion method.
The implementation of the AddVersion method looks good and is consistent with the existing Add method. However, consider the following improvements:
- Add a check to ensure the parent asset exists before creating a new version.
- Validate the
parentparameter to ensure it's not empty or invalid. - Handle potential conflicts if a version with the same filename already exists.
Here's a suggested implementation with these improvements:
func (d *database) AddVersion(ctx context.Context,
accountID xid.ID,
parent AssetID,
filename Filename,
size int,
) (*Asset, error) {
if parent == "" {
return nil, fault.New("parent asset ID cannot be empty")
}
// Check if parent asset exists
_, err := d.GetByID(ctx, parent)
if err != nil {
return nil, fault.Wrap(err, fctx.With(ctx), ftag.With(ftag.InvalidArgument))
}
// Check if a version with the same filename already exists
existing, err := d.Get(ctx, filename)
if err == nil && existing != nil {
return nil, fault.New("asset version already exists").
With(fctx.With(ctx), ftag.With(ftag.AlreadyExists))
}
asset, err := d.db.Asset.
Create().
SetID(filename.GetID()).
SetParentAssetID(parent).
SetFilename(filename.name).
SetSize(size).
SetAccountID(xid.ID(accountID)).
Save(ctx)
if err != nil {
return nil, fault.Wrap(err, fctx.With(ctx), ftag.With(ftag.Internal))
}
return FromModel(asset), nil
}This implementation adds validation for the parent asset ID, checks if the parent asset exists, and handles potential conflicts with existing versions.
To ensure the parent asset check is working correctly, we can verify the usage of the GetByID method:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of GetByID method in the codebase
# Test: Search for the GetByID method usage
rg --type go -A 5 'GetByID\('
Length of output: 20451
Script:
#!/bin/bash
# Description: Check if AddVersion method in app/resources/asset/db.go calls GetByID
rg --type go 'func \(d \*database\) AddVersion.*\{([^}]*)GetByID\(' app/resources/asset/db.go
Length of output: 95
| @@ -99,6 +102,7 @@ func serialiseAsset(a asset.Asset) openapi.Asset { | |||
| Id: a.ID.String(), | |||
| Filename: a.Name.String(), | |||
| Path: path, | |||
| Parent: opt.Map(a.Parent, serialiseAsset).Ptr(), | |||
There was a problem hiding this comment.
💡 Codebase verification
Potential Issue: Recursive Calls in serialiseAsset Function
The serialiseAsset function currently calls itself recursively:
return serialiseAsset(*a)While this allows for the serialization of parent assets, it poses a risk of stack overflow or excessive response sizes if assets are deeply nested.
Recommendation: Implement a depth limit for recursion or consider refactoring to an iterative approach to ensure stability and performance.
🔗 Analysis chain
LGTM! Consider handling for deeply nested assets.
The addition of the Parent field in the serialiseAsset function successfully includes parent asset information in the API response. The use of opt.Map correctly handles cases where there might not be a parent.
However, the recursive call to serialiseAsset for the parent could potentially lead to issues with deeply nested assets. Consider implementing a depth limit to prevent potential stack overflow or excessive response sizes.
To verify the current usage and potential impact, you can run the following script:
This script will help identify where assets are referenced and if there are any existing patterns of recursive asset handling that might need attention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for nested asset references and their depth
# Search for asset references in the codebase
echo "Searching for asset references:"
rg --type go -n 'Parent.*Asset'
# Check for potential recursive calls in asset-related functions
echo "\nChecking for potential recursive calls:"
rg --type go -n 'func.*Asset.*\{' -A 10 | rg 'Parent'
Length of output: 37613
Script:
#!/bin/bash
# Description: Check for recursive calls to serialiseAsset in the codebase
echo "Searching for recursive calls within serialiseAsset function:"
rg --type go 'func\s+serialiseAsset\s*\(' -A 20 | rg 'serialiseAsset\s*\('
Length of output: 365
| const accepted = acceptedMIMEs.reduce((prev: string[], curr: string) => { | ||
| const extensions = mime[curr]?.extensions; | ||
| if (!extensions) { | ||
| return prev; | ||
| } | ||
|
|
||
| return [...prev, ...extensions]; | ||
| }, []); |
There was a problem hiding this comment.
Optimize reduce operation for better performance
The current implementation uses spread syntax inside the reduce function, which can lead to O(n²) time complexity. Although this operation is only performed when a file is rejected, optimizing it would be beneficial for edge cases with many file types.
Consider refactoring the reduce operation to avoid using spread syntax:
- const accepted = acceptedMIMEs.reduce((prev: string[], curr: string) => {
- const extensions = mime[curr]?.extensions;
- if (!extensions) {
- return prev;
- }
-
- return [...prev, ...extensions];
- }, []);
+ const accepted = acceptedMIMEs.reduce((prev: string[], curr: string) => {
+ const extensions = mime[curr]?.extensions;
+ if (extensions) {
+ prev.push(...extensions);
+ }
+ return prev;
+ }, []);This change maintains the same functionality while improving performance by avoiding the creation of new arrays in each iteration.
📝 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 accepted = acceptedMIMEs.reduce((prev: string[], curr: string) => { | |
| const extensions = mime[curr]?.extensions; | |
| if (!extensions) { | |
| return prev; | |
| } | |
| return [...prev, ...extensions]; | |
| }, []); | |
| const accepted = acceptedMIMEs.reduce((prev: string[], curr: string) => { | |
| const extensions = mime[curr]?.extensions; | |
| if (extensions) { | |
| prev.push(...extensions); | |
| } | |
| return prev; | |
| }, []); |
🧰 Tools
🪛 Biome
[error] 68-68: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
| // QueryPrimaryImage chains the current query on the "primary_image" edge. | ||
| func (nq *NodeQuery) QueryPrimaryImage() *AssetQuery { | ||
| query := (&AssetClient{config: nq.config}).Query() | ||
| query.path = func(ctx context.Context) (fromU *sql.Selector, err error) { | ||
| if err := nq.prepareQuery(ctx); err != nil { | ||
| return nil, err | ||
| } | ||
| selector := nq.sqlQuery(ctx) | ||
| if err := selector.Err(); err != nil { | ||
| return nil, err | ||
| } | ||
| step := sqlgraph.NewStep( | ||
| sqlgraph.From(node.Table, node.FieldID, selector), | ||
| sqlgraph.To(asset.Table, asset.FieldID), | ||
| sqlgraph.Edge(sqlgraph.M2O, false, node.PrimaryImageTable, node.PrimaryImageColumn), | ||
| ) | ||
| fromU = sqlgraph.SetNeighbors(nq.driver.Dialect(), step) | ||
| return fromU, nil | ||
| } | ||
| return query | ||
| } |
There was a problem hiding this comment.
Fix the edge definition in QueryPrimaryImage method.
The implementation of the QueryPrimaryImage method looks correct overall. However, there's still a potential issue with the edge definition that needs to be addressed.
The boolean argument in sqlgraph.Edge() should be true for many-to-one edges to indicate the edge's direction. Currently, it's set to false, which may cause incorrect query behavior.
Please apply the following fix:
func (nq *NodeQuery) QueryPrimaryImage() *AssetQuery {
// ... (other code remains the same)
step := sqlgraph.NewStep(
sqlgraph.From(node.Table, node.FieldID, selector),
sqlgraph.To(asset.Table, asset.FieldID),
- sqlgraph.Edge(sqlgraph.M2O, false, node.PrimaryImageTable, node.PrimaryImageColumn),
+ sqlgraph.Edge(sqlgraph.M2O, true, node.PrimaryImageTable, node.PrimaryImageColumn),
)
// ... (other code remains the same)
}📝 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.
| // QueryPrimaryImage chains the current query on the "primary_image" edge. | |
| func (nq *NodeQuery) QueryPrimaryImage() *AssetQuery { | |
| query := (&AssetClient{config: nq.config}).Query() | |
| query.path = func(ctx context.Context) (fromU *sql.Selector, err error) { | |
| if err := nq.prepareQuery(ctx); err != nil { | |
| return nil, err | |
| } | |
| selector := nq.sqlQuery(ctx) | |
| if err := selector.Err(); err != nil { | |
| return nil, err | |
| } | |
| step := sqlgraph.NewStep( | |
| sqlgraph.From(node.Table, node.FieldID, selector), | |
| sqlgraph.To(asset.Table, asset.FieldID), | |
| sqlgraph.Edge(sqlgraph.M2O, false, node.PrimaryImageTable, node.PrimaryImageColumn), | |
| ) | |
| fromU = sqlgraph.SetNeighbors(nq.driver.Dialect(), step) | |
| return fromU, nil | |
| } | |
| return query | |
| } | |
| // QueryPrimaryImage chains the current query on the "primary_image" edge. | |
| func (nq *NodeQuery) QueryPrimaryImage() *AssetQuery { | |
| query := (&AssetClient{config: nq.config}).Query() | |
| query.path = func(ctx context.Context) (fromU *sql.Selector, err error) { | |
| if err := nq.prepareQuery(ctx); err != nil { | |
| return nil, err | |
| } | |
| selector := nq.sqlQuery(ctx) | |
| if err := selector.Err(); err != nil { | |
| return nil, err | |
| } | |
| step := sqlgraph.NewStep( | |
| sqlgraph.From(node.Table, node.FieldID, selector), | |
| sqlgraph.To(asset.Table, asset.FieldID), | |
| sqlgraph.Edge(sqlgraph.M2O, true, node.PrimaryImageTable, node.PrimaryImageColumn), | |
| ) | |
| fromU = sqlgraph.SetNeighbors(nq.driver.Dialect(), step) | |
| return fromU, nil | |
| } | |
| return query | |
| } |
| PrimaryImage *Asset `json:"primary_image,omitempty"` | ||
| Recomentations DatagraphItemList `json:"recomentations"` | ||
|
|
There was a problem hiding this comment.
LGTM: PrimaryImage addition, but there's a typo nearby
The addition of the PrimaryImage *Asset field is consistent with the PR's objective and earlier changes.
However, there's a typo in the nearby Recomentations field. It should be corrected:
- Recomentations DatagraphItemList `json:"recomentations"`
+ Recommendations DatagraphItemList `json:"recommendations"`📝 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.
| PrimaryImage *Asset `json:"primary_image,omitempty"` | |
| Recomentations DatagraphItemList `json:"recomentations"` | |
| PrimaryImage *Asset `json:"primary_image,omitempty"` | |
| Recommendations DatagraphItemList `json:"recommendations"` | |
| await nodeUpdate(slug, { | ||
| primary_image_asset_id: null, | ||
| // NOTE: We don't have access to the original node's meta, so we can't | ||
| // remove the old cover config, but it doesn't really matter. | ||
| }); |
There was a problem hiding this comment.
Add error handling for nodeUpdate in removeNodeCoverImage
The removeNodeCoverImage function does not handle errors that might occur during the nodeUpdate API call. If the API request fails, the application may end up in an inconsistent state. Consider adding error handling to manage possible exceptions.
Apply the following changes to implement error handling:
+ try {
await nodeUpdate(slug, {
primary_image_asset_id: null,
// NOTE: We don't have access to the original node's meta, so we can't
// remove the old cover config, but it doesn't really matter.
});
+ } catch (error) {
+ // Handle the error appropriately, e.g., show a notification or revert changes
+ console.error("Failed to remove node cover image:", error);
+ // Optional: Revert the optimistic update if necessary
+ await mutate(nodeKeyFn);
+ }📝 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.
| await nodeUpdate(slug, { | |
| primary_image_asset_id: null, | |
| // NOTE: We don't have access to the original node's meta, so we can't | |
| // remove the old cover config, but it doesn't really matter. | |
| }); | |
| try { | |
| await nodeUpdate(slug, { | |
| primary_image_asset_id: null, | |
| // NOTE: We don't have access to the original node's meta, so we can't | |
| // remove the old cover config, but it doesn't really matter. | |
| }); | |
| } catch (error) { | |
| // Handle the error appropriately, e.g., show a notification or revert changes | |
| console.error("Failed to remove node cover image:", error); | |
| // Optional: Revert the optimistic update if necessary | |
| await mutate(nodeKeyFn); | |
| } |
| ...newNode, | ||
| primary_image_asset_id: cover?.asset.id, | ||
| // NOTE: We don't have access to the original node's meta, so we have to | ||
| // fully replace it. Right now no other features use metadata, but this | ||
| // will need to be fixed eventually. Probably by either calling the API | ||
| // within this hook to fetch the latest version of the node and spreading. | ||
| meta: newMeta, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Add error handling for nodeUpdate in updateNode
The updateNode function lacks error handling for the nodeUpdate API call. Without proper error management, a failed API request could lead to inconsistent application state. Consider wrapping the API call in a try-catch block to handle potential exceptions.
Here's how you can implement error handling:
+ try {
await nodeUpdate(slug, {
...newNode,
primary_image_asset_id: cover?.asset.id,
meta: newMeta,
});
+ } catch (error) {
+ // Handle the error appropriately, e.g., show a notification or revert changes
+ console.error("Failed to update node:", error);
+ // Optional: Revert the optimistic update if necessary
+ await mutate(nodeKeyFn);
+ }Committable suggestion was skipped due to low confidence.
| const newMeta = | ||
| cover && !cover.isReplacement | ||
| ? ({ | ||
| // TODO: Spread original node metadata here | ||
| coverImage: cover.config, | ||
| } satisfies NodeMetadata) | ||
| : undefined; | ||
|
|
||
| await nodeUpdate(slug, { | ||
| name: newNode.name, | ||
| slug: newNode.slug, | ||
| asset_ids: newNode.asset_ids, | ||
| url: newNode.url, | ||
| content: newNode.content, | ||
| meta: newNode.meta, | ||
| ...newNode, | ||
| primary_image_asset_id: cover?.asset.id, | ||
| // NOTE: We don't have access to the original node's meta, so we have to | ||
| // fully replace it. Right now no other features use metadata, but this | ||
| // will need to be fixed eventually. Probably by either calling the API | ||
| // within this hook to fetch the latest version of the node and spreading. | ||
| meta: newMeta, |
There was a problem hiding this comment.
Avoid overwriting existing metadata when updating the node
In the updateNode function, the existing meta field is completely overwritten by newMeta, which may lead to loss of any additional metadata stored in the node. To prevent potential data loss, consider merging newMeta with the existing meta.
You can apply this diff to merge the existing metadata:
await nodeUpdate(slug, {
...newNode,
primary_image_asset_id: cover?.asset.id,
- meta: newMeta,
+ meta: {
+ ...data.meta,
+ ...newMeta,
+ },
});📝 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 newMeta = | |
| cover && !cover.isReplacement | |
| ? ({ | |
| // TODO: Spread original node metadata here | |
| coverImage: cover.config, | |
| } satisfies NodeMetadata) | |
| : undefined; | |
| await nodeUpdate(slug, { | |
| name: newNode.name, | |
| slug: newNode.slug, | |
| asset_ids: newNode.asset_ids, | |
| url: newNode.url, | |
| content: newNode.content, | |
| meta: newNode.meta, | |
| ...newNode, | |
| primary_image_asset_id: cover?.asset.id, | |
| // NOTE: We don't have access to the original node's meta, so we have to | |
| // fully replace it. Right now no other features use metadata, but this | |
| // will need to be fixed eventually. Probably by either calling the API | |
| // within this hook to fetch the latest version of the node and spreading. | |
| meta: newMeta, | |
| const newMeta = | |
| cover && !cover.isReplacement | |
| ? ({ | |
| // TODO: Spread original node metadata here | |
| coverImage: cover.config, | |
| } satisfies NodeMetadata) | |
| : undefined; | |
| await nodeUpdate(slug, { | |
| ...newNode, | |
| primary_image_asset_id: cover?.asset.id, | |
| // NOTE: We don't have access to the original node's meta, so we have to | |
| // fully replace it. Right now no other features use metadata, but this | |
| // will need to be fixed eventually. Probably by either calling the API | |
| // within this hook to fetch the latest version of the node and spreading. | |
| meta: { | |
| ...data.meta, | |
| ...newMeta, | |
| }, |
| primary_image_asset_id: null, | ||
| // NOTE: We don't have access to the original node's meta, so we can't | ||
| // remove the old cover config, but it doesn't really matter. | ||
| }); |
There was a problem hiding this comment.
Ensure consistent metadata when removing the cover image
When removing the cover image in removeNodeCoverImage, the meta field's coverImage configuration is not updated, potentially leading to inconsistencies if meta.coverImage still exists after the primary_image is removed. Consider updating the meta field to remove the coverImage configuration.
Modify the nodeUpdate call to clear the coverImage from metadata:
await nodeUpdate(slug, {
primary_image_asset_id: null,
+ meta: {
+ ...data.meta,
+ coverImage: null,
+ },
// NOTE: We don't have access to the original node's meta, so we can't
// remove the old cover config, but it doesn't really matter.
});Ensure that you have access to the current meta data when making this update.
Committable suggestion was skipped due to low confidence.
| // PrimaryAssetID holds the value of the "primary_asset_id" field. | ||
| PrimaryAssetID *xid.ID `json:"primary_asset_id,omitempty"` |
There was a problem hiding this comment.
Inconsistent ID field type: Consider consistency for PrimaryAssetID
The PrimaryAssetID field is declared as a pointer to xid.ID (*xid.ID), whereas other ID fields like ParentNodeID, AccountID, and LinkID are declared as xid.ID. For consistency and to avoid potential nil pointer dereferences, consider declaring PrimaryAssetID as xid.ID if it cannot be null. If it can be null, ensure that null values are properly handled throughout the codebase.
This adds a separate asset relation to library pages: primary image, or "cover image" which acts as a hero section header for library pages.
This also introduces the concept of assets having version history. The need for this was because when a cover image is cropped, the original is still needed in order to facilitate future edits to the crop.
from discord:
this, it's a fixed height but obviously dynamic width, so you can drop an image in and move it around a bit so it's positioned nicely
hard part: because the width is dynamic, the cropped coordinates are invalid for that use-case
img has object-fit/object-position, but the coordinate space is different because it's dependent screen space coordinates not image space coordinates
so I think the only solution is to store two images, one is the original and the other is the cropped version (which can be deleted/replaced when the original is edited)
when you enter edit mode, it loads the original image and when it's rendered for reading, it loads the cropped image
So now I'm wondering if it makes sense to add a recursive backreference to Assets for change versioning :thinkthonk: