feat(text-editor): add metadata event for notifying about metadata changes (images and links)#3547
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough""" WalkthroughThis update refactors the text editor's handling of image and link metadata. It introduces a new metadata change event that tracks changes to both images and links, replacing the previous image removal event. The ProseMirror adapter and text editor components now maintain and emit a unified metadata object, and new utility functions are added to extract and compare metadata from the document. The plugin responsible for image insertion is simplified, removing its internal state for tracking image removals. The update also introduces new type definitions and tests for the metadata utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TextEditor
participant ProseMirrorAdapter
participant MetadataUtils
User->>TextEditor: Edit content (add/remove images/links)
TextEditor->>ProseMirrorAdapter: Update document
ProseMirrorAdapter->>MetadataUtils: getMetadataFromDoc(doc)
MetadataUtils-->>ProseMirrorAdapter: MetadataInfo (images, links)
ProseMirrorAdapter->>TextEditor: metadataChange event (MetadataInfo)
TextEditor->>User: metadataChange event (MetadataInfo)
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds metadata extraction functionality to the text editor and updates related events and types accordingly. Key changes include:
- A new utility module for extracting metadata (images and links) from a ProseMirror document.
- Updates to event handling in TextEditor and ProsemirrorAdapter to support a new metadataChange event, with deprecation of imageRemoved.
- Adjustments in tests, type definitions, and plugins (link and image inserter) to align with the new metadata and event model.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/text-editor/utils/metadata-utils.ts | New metadata extraction utilities ensuring order-insensitive comparison and frequency mapping. |
| src/components/text-editor/utils/metadata-utils.spec.ts | Comprehensive tests verifying metadata extraction and comparison. |
| src/components/text-editor/text-editor.types.ts | Added LinkInfo and MetadataInfo interfaces. |
| src/components/text-editor/text-editor.tsx | Updated event handling, deprecating imageRemoved and adding metadataChange event. |
| src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx | Extended adapter to emit metadata changes and remove cached images accordingly. |
| src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts | Modified event detail type from EditorLinkMenuEventDetail to LinkInfo. |
| src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts | Removed state management for removed images to streamline the image insertion logic. |
| src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx | Updated example to use metadataChange instead of imageRemoved and added logic to process removed images. |
Comments suppressed due to low confidence (1)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx:116
- [nitpick] The getRemovedImages method duplicates similar removal logic seen in ProsemirrorAdapter; consider abstracting this functionality into a shared utility to improve maintainability and reduce duplication.
private handleMetadataChange = (event: LimelTextEditorCustomEvent<MetadataInfo>) => {
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
Outdated
Show resolved
Hide resolved
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts
Show resolved
Hide resolved
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx(3 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts(2 hunks)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx(8 hunks)src/components/text-editor/text-editor.tsx(4 hunks)src/components/text-editor/text-editor.types.ts(2 hunks)src/components/text-editor/utils/metadata-utils.spec.ts(1 hunks)src/components/text-editor/utils/metadata-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`**/*.{ts,tsx}`: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
**/*.{ts,tsx}: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.tssrc/components/text-editor/text-editor.types.tssrc/components/text-editor/text-editor.tsxsrc/components/text-editor/utils/metadata-utils.spec.tssrc/components/text-editor/prosemirror-adapter/plugins/image/inserter.tssrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/utils/metadata-utils.tssrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`**/*.tsx`: Our `.tsx` files are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from the `render` method, they will sometime...
**/*.tsx: Our.tsxfiles are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from therendermethod, they will sometimes wrap them in an array literal. In these cases, rather than recommending they addkeyproperties to the elements, recommend removing the hardcoded array literal. Recommend wrapping the elements in StencilJS's special<Host>element.
src/components/text-editor/text-editor.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`**/*.{tsx,scss}`: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
**/*.{tsx,scss}: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
src/components/text-editor/text-editor.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`src/components/**/examples/**/*.{ts,tsx}`: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is p...
src/components/**/examples/**/*.{ts,tsx}: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is publically exported by lime-elements, the import should be made from@limetech/lime-elements. If they import something from another file inside theexamplefolder, the import should use a relative path.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
🧠 Learnings (1)
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
Learnt from: FredrikWallstrom
PR: Lundalogik/lime-elements#3464
File: src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts:58-67
Timestamp: 2025-03-04T14:44:40.706Z
Learning: In the image inserter plugin for the text editor's ProseMirror adapter, only standard image nodes (where node.type.name === 'image') are intentionally handled, as custom image-like nodes are not within the current scope.
🧬 Code Graph Analysis (5)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
src/components/text-editor/text-editor.types.ts (1)
LinkInfo(125-135)
src/components/text-editor/text-editor.tsx (2)
src/interface.ts (1)
EventEmitter(16-16)src/components/text-editor/text-editor.types.ts (2)
ImageInfo(106-120)MetadataInfo(181-191)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (1)
src/components/text-editor/text-editor.types.ts (2)
MetadataInfo(181-191)ImageInfo(106-120)
src/components/text-editor/utils/metadata-utils.ts (1)
src/components/text-editor/text-editor.types.ts (3)
MetadataInfo(181-191)ImageInfo(106-120)LinkInfo(125-135)
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (5)
src/components/text-editor/text-editor.types.ts (3)
MetadataInfo(181-191)ImageInfo(106-120)LinkInfo(125-135)src/interface.ts (1)
EventEmitter(16-16)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
createImageInserterPlugin(12-28)src/components/text-editor/utils/metadata-utils.ts (2)
getMetadataFromDoc(13-38)hasMetadataChanged(44-66)src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (1)
imageCache(7-7)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Lint
🔇 Additional comments (33)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
7-7: Import aligns with repo guidelines – good changeThe shared
LinkInfointerface is imported through a relative path, so the production build will not choke on an absolute import. 👍src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
1-5: Import clean-up looks correct; verify path depth
../../../text-editor.typesis the proper three-level hop fromplugins/imageback to thetext-editorroot, so the relative-import rule is honoured. The removed unused imports also slim the bundle – nice.src/components/text-editor/text-editor.tsx (6)
11-11: LGTM! Added MetadataInfo to imports.Added the new
MetadataInfotype import needed for the metadata change event.
190-191: Good deprecation notice for the imageRemoved event.The deprecation notice clearly indicates that the event is deprecated and provides a migration path by directing users to use the new
metadataChangeevent instead.
196-203: LGTM! New metadataChange event emitter implemented properly.The new event emitter for tracking both images and links metadata changes is well documented with appropriate JSDoc comments and privacy modifiers.
282-284: LGTM! Added new metadata change event listener.The component correctly listens to both the deprecated
onImageRemovedevent (with appropriate warning suppression) and the newonMetadataChangeevent.
347-350: LGTM! New handleMetadataChange method implemented correctly.The method properly stops event propagation and emits the metadata change event.
352-356: LGTM! Updated imageRemoved handler with warning suppression.The handler now includes warning suppression for the deprecated
imageRemovedemitter.src/components/text-editor/utils/metadata-utils.spec.ts (11)
1-36: LGTM! Well-structured test setup.The test setup code creates a proper ProseMirror schema with document, paragraph, text, image nodes, and link mark, which provides a good foundation for testing the metadata utilities.
37-74: LGTM! Comprehensive test for image extraction.This test properly verifies that the
getMetadataFromDocfunction correctly extracts image metadata including src, fileInfoId, and state attributes.
76-122: LGTM! Thorough test for link extraction.The test verifies that
getMetadataFromDoccorrectly extracts link metadata including href and text from marked text nodes.
124-167: LGTM! Good complex document test case.Tests extraction of both images and links from a more complex document structure, ensuring the function handles mixed content correctly.
169-186: LGTM! Test for image count changes.Properly tests that
hasMetadataChangedreturns true when the number of images differs between old and new metadata.
188-203: LGTM! Test for link count changes.Properly tests that
hasMetadataChangedreturns true when the number of links differs between old and new metadata.
205-221: LGTM! Test for image content changes.Properly tests that
hasMetadataChangedreturns true when image content differs between old and new metadata.
223-235: LGTM! Test for link content changes.Properly tests that
hasMetadataChangedreturns true when link text differs between old and new metadata.
237-261: LGTM! Test for duplicate elements handling.Good test case ensuring that duplicate images and links are handled correctly in the metadata comparison.
263-287: LGTM! Test for order independence.Properly verifies that
hasMetadataChangedis order-insensitive, returning false when only the order of elements differs.
289-305: LGTM! Test for identical metadata.Verifies that
hasMetadataChangedreturns false when metadata is identical, which is an important base case.src/components/text-editor/utils/metadata-utils.ts (5)
1-38: LGTM! Well-implemented metadata extraction function.The
getMetadataFromDocfunction efficiently traverses the document tree once and collects both image and link metadata in a single pass. The documentation is clear and the implementation is straightforward.
40-66: LGTM! Efficient metadata comparison function.The
hasMetadataChangedfunction follows a good optimization pattern:
- First checks counts (most efficient test)
- Then performs detailed comparisons using frequency maps
- Returns early when possible
The comments about handling duplicates and being order-insensitive are helpful.
68-81: LGTM! Clean implementation of image frequency map generation.The
getImageFrequencyMapfunction correctly creates a frequency map using a compound key that captures all the important properties of an image.
83-96: LGTM! Clean implementation of link frequency map generation.The
getLinkFrequencyMapfunction follows the same pattern as the image frequency map function, creating keys from href and text properties.
98-116: LGTM! Good map comparison implementation.The
areFrequencyMapsEqualfunction efficiently compares two maps by:
- First checking sizes (fast fail)
- Then comparing key-value pairs one by one
- Returning early if any difference is found
This is an optimal approach for map comparison.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (6)
7-9: LGTM! Added necessary imports.Added imports for
MetadataInfoandImageStatefrom lime-elements package to support the new metadata handling functionality.
46-46: LGTM! Added metadata tracking property.Added a private property to store the current metadata state with proper initialization of empty arrays for both images and links.
55-55: LGTM! Updated event binding in render method.Replaced the deprecated
onImageRemovedwith the newonMetadataChangeevent binding in the text-editor component.
116-133: LGTM! Well-implemented metadata change handler.The new
handleMetadataChangemethod:
- Identifies removed images by comparing old and new metadata
- Logs each removal
- Processes removals of successfully uploaded images
- Updates the stored metadata
This is a good implementation of the new metadata-based approach for handling image removals.
135-146: LGTM! Clear helper method for detecting removed images.The
getRemovedImagesmethod efficiently identifies images that existed in the old metadata but are no longer present in the new metadata by filtering based on fileInfoId.
147-155: LGTM! Updated error handling in removeImage method.The
removeImagemethod now takes anImageInfoparameter and properly references the fileInfoId in the error message when removal fails.src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (3)
422-425: Double-check callback binding forimagePasted
createImageInserterPluginexpects a callback of typeImagePastedCallback.
Passingthis.imagePasted.emitworks only becauseEventEmitter.emitis an arrow
function that is already bound to its instance.
If the implementation ofEventEmitterever changes (or if someone replaces it in a test),
thethiscontext could be lost.A future-proof pattern is to wrap the call explicitly:
- createImageInserterPlugin(this.imagePasted.emit), + createImageInserterPlugin((payload) => this.imagePasted.emit(payload)),The wrapper is negligible in cost and guarantees the correct context regardless of
internal changes.
501-525: Guard against missingfileInfoIdwhen cleaning the cache
removeImagesFromCacheassumes every image possesses afileInfoId.
If an upstream plugin forgets to set this attribute, the method will
attempt todelete(undefined)and emit an event containing an invalid
identifier.- imageCache.delete(image.fileInfoId); - this.imageRemoved.emit(image); + if (image.fileInfoId) { + imageCache.delete(image.fileInfoId); + this.imageRemoved.emit(image); + }Adding the check prevents silent failures and keeps the cache in a consistent
state.
192-200: Great addition – newmetadataChangeevent is well-documentedThe consolidated event simplifies external consumers’ logic and the deprecation
notice forimageRemovedis clear. Nice work!
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts
Outdated
Show resolved
Hide resolved
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3547/ |
FredrikWallstrom
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
137-141: Event-payload construction can be simplified & made future-proofYou can tighten this block a little and make
openLinkMenueasier to evolve by:
- Passing the whole
LinkInfoobject rather than two primitives – avoids parameter explosion when more link fields are added.- Using property-shorthand to reduce noise.
-const openLinkMenu = (view: EditorView, href: string, text: string) => { - const event = new CustomEvent<LinkInfo>('open-editor-link-menu', { - detail: { href: href, text: text }, +const openLinkMenu = (view: EditorView, link: LinkInfo) => { + const event = new CustomEvent<LinkInfo>('open-editor-link-menu', { + detail: link, bubbles: true, composed: true, }); view.dom.dispatchEvent(event); }Call-site update (single place → minimal churn):
- openLinkMenu(view, href, text); + openLinkMenu(view, { href, text });Small, but it keeps the emit API symmetrical with the interface definition.
src/components/text-editor/text-editor.types.ts (2)
122-135:LinkInfolooks solid, minor doc tweak possibleThe structure is clear. Consider adding an example in the JSDoc showing how it's used, which helps consumers quickly understand intent.
No functional changes required.
175-191: Consider immutability forMetadataInfocollections
imagesandlinksare exposed as mutable arrays. If external code accidentally mutates them the internal diffing logic could be bypassed.Two easy wins:
-export interface MetadataInfo { +export interface MetadataInfo { ... - images: ImageInfo[]; + readonly images: ReadonlyArray<ImageInfo>; ... - links: LinkInfo[]; + readonly links: ReadonlyArray<LinkInfo>; }This keeps metadata snapshots immutable and sidesteps accidental side-effects.
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (3)
472-475: 🛠️ Refactor suggestionPotential performance issue with metadata extraction.
The code extracts metadata from the entire document after each update, which could be inefficient for large documents.
Consider implementing one of these optimizations to avoid full-document scans on every update:
- Extract metadata incrementally from transaction steps
- Debounce the metadata extraction
- Memoize the last parsed document reference to skip extraction when unchanged
- const metadata = getMetadataFromDoc(this.view.state.doc); - this.metadataEmitter(metadata); + this.scheduleMetadataUpdate(this.view.state.doc);Where
scheduleMetadataUpdatewould implement appropriate optimizations.
493-495: 🛠️ Refactor suggestionSame performance issue in transaction handling.
Similar to the previous comment, extracting metadata on every transaction could impact performance.
Apply the same optimization here as suggested for lines 472-475 to avoid redundant document scans.
501-507: 🧹 Nitpick (assertive)Add inline documentation to metadataEmitter.
The metadataEmitter method would benefit from more detailed comments explaining the rationale behind the frequency map comparison and order-insensitive change detection.
private metadataEmitter(metadata: MetadataInfo) { + // Check if the metadata has changed using a frequency map comparison. + // This ensures that changes are detected regardless of the order of items in the metadata. if (hasMetadataChanged(this.metadata, metadata)) { + // Remove images from the cache that are no longer present in the new metadata. this.removeImagesFromCache(metadata, this.metadata); + // Update the current metadata and emit the metadataChange event + // to notify listeners about the updated metadata. this.metadata = metadata; this.metadataChange.emit(metadata); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (8)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx(3 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts(2 hunks)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx(8 hunks)src/components/text-editor/text-editor.tsx(4 hunks)src/components/text-editor/text-editor.types.ts(2 hunks)src/components/text-editor/utils/metadata-utils.spec.ts(1 hunks)src/components/text-editor/utils/metadata-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`**/*.{ts,tsx}`: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
**/*.{ts,tsx}: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.tssrc/components/text-editor/text-editor.types.tssrc/components/text-editor/text-editor.tsxsrc/components/text-editor/utils/metadata-utils.tssrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/utils/metadata-utils.spec.tssrc/components/text-editor/prosemirror-adapter/plugins/image/inserter.tssrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`**/*.tsx`: Our `.tsx` files are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from the `render` method, they will sometime...
**/*.tsx: Our.tsxfiles are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from therendermethod, they will sometimes wrap them in an array literal. In these cases, rather than recommending they addkeyproperties to the elements, recommend removing the hardcoded array literal. Recommend wrapping the elements in StencilJS's special<Host>element.
src/components/text-editor/text-editor.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`**/*.{tsx,scss}`: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
**/*.{tsx,scss}: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
src/components/text-editor/text-editor.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`src/components/**/examples/**/*.{ts,tsx}`: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is p...
src/components/**/examples/**/*.{ts,tsx}: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is publically exported by lime-elements, the import should be made from@limetech/lime-elements. If they import something from another file inside theexamplefolder, the import should use a relative path.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
🧠 Learnings (1)
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
Learnt from: FredrikWallstrom
PR: Lundalogik/lime-elements#3464
File: src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts:58-67
Timestamp: 2025-03-04T14:44:40.706Z
Learning: In the image inserter plugin for the text editor's ProseMirror adapter, only standard image nodes (where node.type.name === 'image') are intentionally handled, as custom image-like nodes are not within the current scope.
🧬 Code Graph Analysis (6)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
src/components/text-editor/text-editor.types.ts (1)
LinkInfo(125-135)
src/components/text-editor/text-editor.tsx (2)
src/interface.ts (1)
EventEmitter(16-16)src/components/text-editor/text-editor.types.ts (2)
ImageInfo(106-120)MetadataInfo(181-191)
src/components/text-editor/utils/metadata-utils.ts (1)
src/components/text-editor/text-editor.types.ts (3)
MetadataInfo(181-191)ImageInfo(106-120)LinkInfo(125-135)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (1)
src/components/text-editor/text-editor.types.ts (2)
MetadataInfo(181-191)ImageInfo(106-120)
src/components/text-editor/utils/metadata-utils.spec.ts (2)
src/components/text-editor/utils/metadata-utils.ts (2)
getMetadataFromDoc(13-38)hasMetadataChanged(44-66)src/components/text-editor/text-editor.types.ts (1)
MetadataInfo(181-191)
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (5)
src/components/text-editor/text-editor.types.ts (3)
MetadataInfo(181-191)ImageInfo(106-120)LinkInfo(125-135)src/interface.ts (1)
EventEmitter(16-16)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
createImageInserterPlugin(12-28)src/components/text-editor/utils/metadata-utils.ts (2)
getMetadataFromDoc(13-38)hasMetadataChanged(44-66)src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (1)
imageCache(7-7)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Lint
🔇 Additional comments (36)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
7-7: Good integration of the shared type system.The import of
LinkInfofrom the shared types file aligns with the unified metadata handling approach, promoting type consistency across the codebase.src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (2)
1-5: Excellent plugin simplification.The removal of state management imports and internal image tracking logic greatly simplifies this plugin, aligning with the move toward centralized metadata handling.
12-14: Clean interface refinement.Removing the
imageRemovedCallbackparameter streamlines the plugin's API to focus solely on image insertion, which is consistent with the broader refactoring for unified metadata handling.src/components/text-editor/text-editor.tsx (6)
11-11: Good import addition for the new metadata model.Import of
MetadataInfoaligns with the introduction of the unified metadata handling approach.
190-192: Well-documented deprecation.The deprecation notice is clear and provides a good migration path for consumers.
196-204: Good addition of the unified metadata event.The new
metadataChangeevent follows the same pattern as existing events and properly expands tracking to include both images and links.
282-284: Proper backward compatibility handling.The code maintains compatibility by keeping the deprecated event while adding the new event, with appropriate ESLint suppressions.
347-351: Clean event handler implementation.The new
handleMetadataChangemethod follows the established pattern of stopping propagation and emitting the event.
353-355: Good ESLint suppression for deprecated code.Properly suppresses the ESLint warning for the deprecated event emission.
src/components/text-editor/utils/metadata-utils.spec.ts (10)
1-3: Good choice of imports.The imports are clean and focused on exactly what's needed for testing the metadata utilities.
5-31: Well-structured test schema.The test schema correctly models the document structure needed for testing the metadata utilities, with appropriate nodes and marks for images and links.
33-35: Useful test helper function.This helper function simplifies the creation of test documents from JSON, making the tests more readable.
37-74: Thorough test for image metadata extraction.The test correctly verifies that
getMetadataFromDocextracts image attributes including src, fileInfoId, and state.
76-122: Comprehensive test for link metadata extraction.The test properly validates that link attributes including href and text content are correctly extracted from the document.
124-167: Good test for complex document handling.This test ensures that the metadata extraction works correctly with documents containing both images and links, which is an important edge case.
169-186: Comprehensive change detection test cases.These tests thoroughly verify the behavior of
hasMetadataChangedfor different scenarios:
- Different image counts
- Different link counts
- Changed image content
- Changed link content
This ensures robust change detection for all metadata types.
Also applies to: 188-203, 205-221, 223-235
237-261: Excellent test for duplicate handling.This test ensures that the comparison logic correctly handles duplicate elements in the metadata, which is an important edge case to cover.
263-287: Good test for order-insensitive comparison.The test verifies that the metadata comparison ignores the order of elements, which is important for reliable change detection.
289-305: Important baseline test for unchanged metadata.This test confirms that
hasMetadataChangedcorrectly returns false when the metadata is identical, establishing a reliable baseline.src/components/text-editor/utils/metadata-utils.ts (6)
1-3: Clean and focused imports.Imports are well-organized and limited to what's necessary.
4-38: Well-implemented metadata extraction function.The
getMetadataFromDocfunction correctly traverses the document tree to extract metadata about images and links. The documentation is clear, and the implementation handles both image nodes and text nodes with link marks appropriately.
40-66: Efficient metadata comparison implementation.The
hasMetadataChangedfunction takes a smart approach by first checking length differences, then comparing frequency maps of links and images. This handles duplicates correctly while being order-insensitive, as noted in the documentation.
68-81: Good frequency map implementation for images.The function correctly creates a unique key for each image based on its properties and tracks the count of each unique image.
83-96: Consistent frequency map implementation for links.Similar to the image frequency map, this function creates a unique key for each link and tracks counts properly.
98-116: Thorough map comparison helper.The
areFrequencyMapsEqualfunction properly checks both the size of the maps and the counts for each key, ensuring an accurate comparison.src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (5)
1-10: Updated imports to support new metadata functionality.The imports now include the necessary
MetadataInfoandImageStatetypes to support the refactored metadata handling.
46-47: Good initialization of metadata state.Adding the metadata state property provides a way to track and compare metadata changes over time.
48-56: Updated event handler to use new metadata change event.The component now listens for the
onMetadataChangeevent instead of the deprecatedonImageRemovedevent, aligning with the new unified metadata approach.
116-133: Well-implemented metadata change handler.The
handleMetadataChangemethod properly identifies removed images by comparing the old and new metadata, and takes appropriate actions for images in the SUCCESS state.
135-145: Effective helper method for identifying removed images.The
getRemovedImagesmethod efficiently finds images that were present in the old metadata but not in the new metadata based onfileInfoId.src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (6)
55-58: Good addition of utility function imports.The imports for
getMetadataFromDocandhasMetadataChangedare properly added to support the new metadata tracking functionality.
186-188: Good deprecation notice for imageRemoved event.The JSDoc comment properly marks the
imageRemovedevent as deprecated and directs users to the newmetadataChangeevent.
192-200: Well-documented new metadataChange event.The JSDoc for the new event clearly explains its purpose and scope.
423-424: Updated plugin initialization.The
createImageInserterPlugincall has been updated to remove the second argument (the deprecated image removal callback), reflecting the architecture change.
509-525: Well-implemented cache cleanup.The
removeImagesFromCachemethod correctly identifies and removes images that are no longer present in the new metadata, and emits the deprecated event for backward compatibility.
583-588: Updated link menu handler.The event handler correctly uses the new
LinkInfotype instead of the removed local interface.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
Outdated
Show resolved
Hide resolved
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
136-143: 🧹 Nitpick (assertive)Consider simplifying the event payload construction as previously recommended.
While you've updated the CustomEvent to use the centralized
LinkInfotype, the function signature still takes separate parameters instead of accepting a singleLinkInfoobject directly.-const openLinkMenu = (view: EditorView, href: string, text: string) => { - const event = new CustomEvent<LinkInfo>('open-editor-link-menu', { - detail: { href: href, text: text }, +const openLinkMenu = (view: EditorView, link: LinkInfo) => { + const event = new CustomEvent<LinkInfo>('open-editor-link-menu', { + detail: link, bubbles: true, composed: true, }); view.dom.dispatchEvent(event); }This would require updating line 169 to:
- openLinkMenu(view, href, text); + openLinkMenu(view, { href, text });src/components/text-editor/text-editor.types.ts (1)
175-191: 🧹 Nitpick (assertive)Consider making MetadataInfo collections immutable.
The
imagesandlinksarrays are currently mutable, which could lead to unintended modifications from external code that might bypass internal diffing logic.export interface MetadataInfo { /** * Collection of image elements found in the document */ - images: ImageInfo[]; + readonly images: ReadonlyArray<ImageInfo>; /** * Collection of link elements found in the document */ - links: LinkInfo[]; + readonly links: ReadonlyArray<LinkInfo>; }This ensures metadata snapshots remain immutable and prevents accidental side-effects.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (1)
147-155: Implementation needed for image removal.The
removeImagemethod currently only logs the removal and has a placeholder error. It needs a proper implementation to handle the actual removal of images from external storage.Replace the placeholder error with actual implementation code for removing images from external storage, or add more detailed comments explaining what a real implementation would need to do.
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (3)
159-159: Initial metadata state.The metadata property is appropriately initialized with empty arrays for images and links.
Consider extracting the default empty metadata to a constant to improve readability and avoid object re-creation, as suggested in a previous review comment:
-private metadata: MetadataInfo = { images: [], links: [] }; +private metadata: MetadataInfo = EMPTY_METADATA;And add at the end of the file:
const EMPTY_METADATA: MetadataInfo = { images: [], links: [] };
473-475: Avoid running full-document scans on every programmatic update.
getMetadataFromDoctraverses the entire ProseMirror document twice:
once inupdateViewand once for every dispatched transaction.
For large documents this can become the dominant cost of typing.Consider one of the following strategies:
- Extract metadata incrementally from the
transaction.stepsyou already receive.- Debounce the call so that it runs at most once every n ms while typing.
- Memoise the last parsed
docreference and skip the extraction when it is unchanged.-const metadata = getMetadataFromDoc(this.view.state.doc); -this.metadataEmitter(metadata); +this.scheduleMetadataUpdate(this.view.state.doc);where
scheduleMetadataUpdateinternally usesrequestIdleCallback/debounce.This retains correctness while eliminating redundant tree walks.
Also applies to: 493-495
501-507: 🧹 Nitpick (assertive)Add comments explaining the metadata emission logic.
Consider adding inline comments within
metadataEmitterto explain the rationale behind comparing metadata and emitting the event; this would help future maintainers understand the order-insensitive change detection logic.private metadataEmitter(metadata: MetadataInfo) { + // Check if the metadata has changed using the utility function that handles + // duplicate elements and is order-insensitive if (hasMetadataChanged(this.metadata, metadata)) { + // Handle cleanup of removed images and emit events this.removeImagesFromCache(metadata, this.metadata); this.metadata = metadata; this.metadataChange.emit(metadata); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (8)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx(3 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts(2 hunks)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx(8 hunks)src/components/text-editor/text-editor.tsx(4 hunks)src/components/text-editor/text-editor.types.ts(2 hunks)src/components/text-editor/utils/metadata-utils.spec.ts(1 hunks)src/components/text-editor/utils/metadata-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`**/*.{ts,tsx}`: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
**/*.{ts,tsx}: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
src/components/text-editor/utils/metadata-utils.spec.tssrc/components/text-editor/prosemirror-adapter/plugins/link-plugin.tssrc/components/text-editor/prosemirror-adapter/plugins/image/inserter.tssrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsxsrc/components/text-editor/utils/metadata-utils.tssrc/components/text-editor/text-editor.types.tssrc/components/text-editor/text-editor.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
`**/*.tsx`: Our `.tsx` files are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from the `render` method, they will sometime...
**/*.tsx: Our.tsxfiles are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from therendermethod, they will sometimes wrap them in an array literal. In these cases, rather than recommending they addkeyproperties to the elements, recommend removing the hardcoded array literal. Recommend wrapping the elements in StencilJS's special<Host>element.
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsxsrc/components/text-editor/text-editor.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
`**/*.{tsx,scss}`: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
**/*.{tsx,scss}: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsxsrc/components/text-editor/text-editor.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
`src/components/**/examples/**/*.{ts,tsx}`: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is p...
src/components/**/examples/**/*.{ts,tsx}: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is publically exported by lime-elements, the import should be made from@limetech/lime-elements. If they import something from another file inside theexamplefolder, the import should use a relative path.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
🧠 Learnings (1)
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
Learnt from: FredrikWallstrom
PR: Lundalogik/lime-elements#3464
File: src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts:58-67
Timestamp: 2025-03-04T14:44:40.706Z
Learning: In the image inserter plugin for the text editor's ProseMirror adapter, only standard image nodes (where node.type.name === 'image') are intentionally handled, as custom image-like nodes are not within the current scope.
🧬 Code Graph Analysis (5)
src/components/text-editor/utils/metadata-utils.spec.ts (2)
src/components/text-editor/utils/metadata-utils.ts (2)
getMetadataFromDoc(13-38)hasMetadataChanged(44-66)src/components/text-editor/text-editor.types.ts (1)
MetadataInfo(181-191)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
src/components/text-editor/text-editor.types.ts (1)
LinkInfo(125-135)
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (5)
src/components/text-editor/text-editor.types.ts (3)
MetadataInfo(181-191)ImageInfo(106-120)LinkInfo(125-135)src/interface.ts (1)
EventEmitter(16-16)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
createImageInserterPlugin(12-28)src/components/text-editor/utils/metadata-utils.ts (2)
getMetadataFromDoc(13-38)hasMetadataChanged(44-66)src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (1)
imageCache(7-7)
src/components/text-editor/utils/metadata-utils.ts (1)
src/components/text-editor/text-editor.types.ts (3)
MetadataInfo(181-191)ImageInfo(106-120)LinkInfo(125-135)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (1)
src/components/text-editor/text-editor.types.ts (2)
MetadataInfo(181-191)ImageInfo(106-120)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Build
🔇 Additional comments (19)
src/components/text-editor/text-editor.types.ts (1)
122-135: The LinkInfo interface looks well-structured.The interface is well-defined with clear JSDoc comments explaining each property.
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (2)
1-5: Simplified imports improve code maintainability.You've successfully removed unnecessary imports and simplified the plugin structure by removing image removal tracking.
12-28: Clean plugin simplification.Great job simplifying the plugin to focus solely on image pasting by removing the
imageRemovedCallbackparameter and related state management. This reduces responsibilities and makes the code more maintainable.src/components/text-editor/text-editor.tsx (6)
7-12: Appropriate import of new MetadataInfo type.Adding the
MetadataInfoimport supports the new metadata change tracking functionality.
190-192: Well-documented deprecation for imageRemoved event.The deprecation notice provides clear guidance to use the new
metadataChangeevent instead.
196-204: Good addition of the new metadataChange event.The event is properly typed with MetadataInfo and includes appropriate JSDoc comments with visibility and stability annotations.
282-283: Proper handling of both legacy and new events in render method.You've maintained backward compatibility by keeping the deprecated event handler while adding the new one, and you've correctly added the ESLint disable comment for the deprecated method.
346-349: Clean implementation of new metadata change handler.The handler follows the established pattern of other event handlers in the component.
351-355: Appropriate ESLint suppression for deprecated method usage.Good job adding the ESLint disable comment to suppress deprecation warnings in your own code.
src/components/text-editor/utils/metadata-utils.spec.ts (1)
1-304: Test suite is comprehensive and well-structured.This new test file provides excellent coverage for the metadata utility functions. The tests cover various scenarios for both
getMetadataFromDocandhasMetadataChanged, including extraction of images, links, complex documents, and change detection with different metadata modifications.I appreciate the thorough testing of edge cases such as duplicate elements and order-insensitive comparison. The test schema construction is also well done, providing a realistic simulation of the ProseMirror environment.
src/components/text-editor/utils/metadata-utils.ts (1)
1-116: Well-implemented metadata utilities with clear separation of concerns.The implementation of
getMetadataFromDocandhasMetadataChangedis clean and efficient. The use of frequency maps to handle duplicate elements and order-insensitivity is a good approach. The helper functions are well-factored and appropriately scoped.src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (3)
55-55: Updated event handler to use the new metadata change event.Good update from
onImageRemovedtoonMetadataChangeto align with the broader refactoring of metadata handling.
116-133: Well-implemented metadata change handler.The
handleMetadataChangemethod efficiently processes metadata changes, identifies removed images, and takes appropriate actions for successful uploads.
135-145: Efficient implementation of getRemovedImages.The helper method effectively uses array methods to find images that exist in the old metadata but not in the new metadata.
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (5)
186-188: Good deprecation notice with migration path.The deprecation comment for
imageRemovedevent is clear and provides helpful guidance on using the newmetadataChangeevent instead.
192-200: Well-documented new event emitter.The
metadataChangeevent is properly documented with appropriate JSDoc comments explaining its purpose and metadata tracking capabilities.
423-423: Updated image inserter plugin usage.The plugin initialization has been simplified to only handle image insertion, removing the image removal tracking which is now handled through the metadata system.
509-525: Well-implemented image cleanup with backward compatibility.The
removeImagesFromCachemethod efficiently identifies removed images, cleans up the cache, and maintains backward compatibility by emitting the deprecated event.
583-583: Updated event parameter type for link menu handling.The
handleOpenLinkMenumethod now correctly uses theLinkInfointerface for the event parameter, aligning with the unified metadata approach.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
137-141: Event-payload construction can be simplified & made future-proofYou can tighten this block and make
openLinkMenueasier to evolve by:
- Passing the whole
LinkInfoobject rather than two primitives – avoids parameter explosion when more link fields are added.- Using property-shorthand to reduce noise.
-const openLinkMenu = (view: EditorView, href: string, text: string) => { - const event = new CustomEvent<LinkInfo>('open-editor-link-menu', { - detail: { href: href, text: text }, +const openLinkMenu = (view: EditorView, link: LinkInfo) => { + const event = new CustomEvent<LinkInfo>('open-editor-link-menu', { + detail: link, bubbles: true, composed: true, }); view.dom.dispatchEvent(event); }src/components/text-editor/text-editor.types.ts (2)
122-135:LinkInfolooks solid, minor doc tweak possibleThe structure is clear. Consider adding an example in the JSDoc showing how it's used, which helps consumers quickly understand intent.
175-191: Consider immutability forMetadataInfocollections
imagesandlinksare exposed as mutable arrays. If external code accidentally mutates them the internal diffing logic could be bypassed.-export interface MetadataInfo { +export interface MetadataInfo { ... - images: ImageInfo[]; + readonly images: ReadonlyArray<ImageInfo>; ... - links: LinkInfo[]; + readonly links: ReadonlyArray<LinkInfo>; }src/components/text-editor/utils/metadata-utils.ts (1)
40-47: JSDoc comment has been expanded as suggested in past review.The detailed JSDoc now includes parameter descriptions and return value description, addressing the previous review comment.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (1)
147-155:⚠️ Potential issueImplementation needed for image removal.
The
removeImagemethod currently only logs the removal and has a placeholder error. It needs a proper implementation to handle the actual removal of images from external storage.Replace the placeholder error with actual implementation code for removing images from external storage, or add more detailed comments explaining what a real implementation would need to do.
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (2)
471-473: 🧹 Nitpick (assertive)Consider optimizing metadata extraction for large documents.
getMetadataFromDoctraverses the entire document on every update, which could be expensive for large documents.While full document traversal is simple and reliable as noted in your response to previous feedback, for performance with large documents consider:
- Debouncing the metadata extraction during rapid editing
- Checking if the document reference has changed before extraction
- Using a more targeted approach based on transaction steps
- const metadata = getMetadataFromDoc(this.view.state.doc); - this.metadataEmitter(metadata); + // Only extract metadata if the document has changed + if (this.lastProcessedDoc !== this.view.state.doc) { + this.lastProcessedDoc = this.view.state.doc; + const metadata = getMetadataFromDoc(this.view.state.doc); + this.metadataEmitter(metadata); + }
499-505: 🧹 Nitpick (assertive)Add explanatory comments to the metadataEmitter method.
The method could benefit from inline comments explaining the detection and notification logic.
private metadataEmitter(metadata: MetadataInfo) { + // Check if metadata has changed using order-insensitive comparison if (hasMetadataChanged(this.metadata, metadata)) { + // Clean up any removed images from cache and emit legacy events this.removeImagesFromCache(metadata, this.metadata); + // Update internal state with new metadata this.metadata = metadata; + // Notify listeners about the metadata change this.metadataChange.emit(metadata); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (8)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx(3 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts(2 hunks)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx(8 hunks)src/components/text-editor/text-editor.tsx(4 hunks)src/components/text-editor/text-editor.types.ts(2 hunks)src/components/text-editor/utils/metadata-utils.spec.ts(1 hunks)src/components/text-editor/utils/metadata-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`**/*.{ts,tsx}`: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
**/*.{ts,tsx}: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.tssrc/components/text-editor/text-editor.types.tssrc/components/text-editor/text-editor.tsxsrc/components/text-editor/utils/metadata-utils.spec.tssrc/components/text-editor/prosemirror-adapter/plugins/image/inserter.tssrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/utils/metadata-utils.tssrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`**/*.tsx`: Our `.tsx` files are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from the `render` method, they will sometime...
**/*.tsx: Our.tsxfiles are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from therendermethod, they will sometimes wrap them in an array literal. In these cases, rather than recommending they addkeyproperties to the elements, recommend removing the hardcoded array literal. Recommend wrapping the elements in StencilJS's special<Host>element.
src/components/text-editor/text-editor.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`**/*.{tsx,scss}`: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
**/*.{tsx,scss}: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
src/components/text-editor/text-editor.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`src/components/**/examples/**/*.{ts,tsx}`: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is p...
src/components/**/examples/**/*.{ts,tsx}: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is publically exported by lime-elements, the import should be made from@limetech/lime-elements. If they import something from another file inside theexamplefolder, the import should use a relative path.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
🧠 Learnings (1)
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
Learnt from: FredrikWallstrom
PR: Lundalogik/lime-elements#3464
File: src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts:58-67
Timestamp: 2025-03-04T14:44:40.706Z
Learning: In the image inserter plugin for the text editor's ProseMirror adapter, only standard image nodes (where node.type.name === 'image') are intentionally handled, as custom image-like nodes are not within the current scope.
🧬 Code Graph Analysis (5)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
src/components/text-editor/text-editor.types.ts (1)
LinkInfo(125-135)
src/components/text-editor/text-editor.tsx (2)
src/interface.ts (1)
EventEmitter(16-16)src/components/text-editor/text-editor.types.ts (2)
ImageInfo(106-120)MetadataInfo(181-191)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (1)
src/components/text-editor/text-editor.types.ts (2)
MetadataInfo(181-191)ImageInfo(106-120)
src/components/text-editor/utils/metadata-utils.ts (1)
src/components/text-editor/text-editor.types.ts (3)
MetadataInfo(181-191)ImageInfo(106-120)LinkInfo(125-135)
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (5)
src/components/text-editor/text-editor.types.ts (2)
MetadataInfo(181-191)LinkInfo(125-135)src/interface.ts (1)
EventEmitter(16-16)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
createImageInserterPlugin(12-28)src/components/text-editor/utils/metadata-utils.ts (2)
getMetadataFromDoc(13-38)hasMetadataChanged(48-70)src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (1)
imageCache(7-7)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Build
- GitHub Check: SonarCloud
🔇 Additional comments (17)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
7-7: Good use of relative imports.The import of
LinkInfofrom a relative path follows the coding guideline for importing from other files within the lime-elements module.src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (2)
1-5: Simplified imports reflect plugin's streamlined responsibility.The removal of
TransactionandStateFieldimports and the direct import ofImageStatefrom the types file properly reflect the plugin's streamlined responsibility - it now focuses on image insertion without maintaining state for tracking removals.
12-28: Plugin simplified to focus on core functionality.The removal of the image tracking callback parameter properly aligns with the new metadata-driven architecture. The plugin now focuses solely on handling paste events and dispatching image pasted events, which is cleaner and more maintainable.
src/components/text-editor/text-editor.tsx (6)
11-11: Good import ofMetadataInfofor unified metadata handling.Properly importing the new type from the shared types file.
190-192: Well-documented deprecation notice.The deprecation notice clearly communicates that the event is being phased out and directs users to the new
metadataChangeevent as an alternative, following good deprecation practices.
196-204: New metadata event follows consistent pattern.The new
metadataChangeevent emitter is properly typed and documented with an@alphatag to indicate its experimental status. The event description clearly explains its purpose for tracking both images and links.
283-283: Added event listener for new metadata change events.The
onMetadataChangeattribute correctly wires up the new event to the appropriate handler.
346-349: Handler follows consistent implementation pattern.The
handleMetadataChangemethod follows the same pattern as other event handlers in the class, stopping propagation and emitting the event with its detail.
351-355: Appropriate suppression of deprecation warning.The eslint suppression comment for the deprecated method is correctly placed and specific to the
sonarjs/deprecationrule, following best practices for handling known deprecations.src/components/text-editor/utils/metadata-utils.spec.ts (1)
1-306: Comprehensive test coverage for metadata utilities.The test suite thoroughly covers the functionality of both utility functions. It includes test cases for various scenarios including different document structures, empty collections, duplicate elements, and order changes.
src/components/text-editor/utils/metadata-utils.ts (1)
72-85: 🧹 Nitpick (assertive)Consider adding type safety for image key generation.
The string concatenation approach for creating frequency map keys works but could be improved with stronger type safety.
-function getImageFrequencyMap(images: ImageInfo[]): Map<string, number> { +function getImageFrequencyMap(images: ImageInfo[]): Map<string, number> { const countMap = new Map<string, number>(); images.forEach((image) => { - const key = `${image.fileInfoId}|${image.state}|${image.src}`; + // Ensure all required fields are present before creating the key + const key = `${image.fileInfoId || ''}|${image.state || ''}|${image.src || ''}`; countMap.set(key, (countMap.get(key) || 0) + 1); }); return countMap; }Likely an incorrect or invalid review comment.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (2)
46-46: Maintaining object literal initialization for metadata is correct.As noted in your past feedback conversation with a reviewer, using a constant would cause issues when the arrays are modified due to JavaScript's reference semantics.
116-133: Well-structured metadata change handler.The component now correctly handles metadata changes by detecting removed images and taking appropriate action. The implementation aligns with the new architecture for metadata tracking.
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (4)
159-160: Maintaining object literal initialization for metadata is correct.Similar to the example component, using an object literal instead of a constant is appropriate here since the arrays will be modified.
190-198: Good use of event emitter for metadata changes.The new event emitter is well-documented with appropriate annotations for API visibility.
421-422: Simplified plugin creation by removing image removal callback.Now that image removal is handled by the metadata system, the second argument to
createImageInserterPluginhas been correctly removed.
507-522: Good backward compatibility with imageRemoved events.The implementation correctly maintains backward compatibility by emitting the deprecated
imageRemovedevents while transitioning to the new metadata change system.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
137-141: Event-payload construction can be simplified & made future-proofYou can tighten this block a little and make
openLinkMenueasier to evolve by:
- Passing the whole
LinkInfoobject rather than two primitives – avoids parameter explosion when more link fields are added.- Using property-shorthand to reduce noise.
-const openLinkMenu = (view: EditorView, href: string, text: string) => { - const event = new CustomEvent<LinkInfo>('open-editor-link-menu', { - detail: { href: href, text: text }, +const openLinkMenu = (view: EditorView, link: LinkInfo) => { + const event = new CustomEvent<LinkInfo>('open-editor-link-menu', { + detail: link, bubbles: true, composed: true, }); view.dom.dispatchEvent(event); }Call-site update (single place → minimal churn):
- openLinkMenu(view, href, text); + openLinkMenu(view, { href, text });Small, but it keeps the emit API symmetrical with the interface definition.
src/components/text-editor/text-editor.types.ts (2)
125-135:LinkInfolooks solid, minor doc tweak possibleThe structure is clear. Consider adding an example in the JSDoc showing how it's used, which helps consumers quickly understand intent.
No functional changes required.
175-191: Consider immutability forMetadataInfocollections
imagesandlinksare exposed as mutable arrays. If external code accidentally mutates them the internal diffing logic could be bypassed.Two easy wins:
-export interface MetadataInfo { +export interface MetadataInfo { ... - images: ImageInfo[]; + readonly images: ReadonlyArray<ImageInfo>; ... - links: LinkInfo[]; + readonly links: ReadonlyArray<LinkInfo>; }This keeps metadata snapshots immutable and sidesteps accidental side-effects.
src/components/text-editor/utils/metadata-utils.ts (1)
90-97: Add null-safe fallbacks when generating link frequency keys
If eitherhrefortextisundefinedthe template string will render"undefined", conflating distinct missing-value cases and breaking the frequency map.- const key = `${link.href}|${link.text}`; + const key = `${link.href ?? ''}|${link.text ?? ''}`;This ensures the key generation is robust even for partially-defined
LinkInfoobjects.src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (1)
147-154: 🧹 Nitpick (assertive)
removeImageis still a stub – implement or document the real behaviourThe method currently throws a “Not implemented” error.
Either implement the actual deletion logic (e.g. an API call to the external storage) or replace the throw with a clear TODO explaining what has to be done in production.src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (1)
471-473: 🧹 Nitpick (assertive)Full-document metadata scan executed on every programmatic update
getMetadataFromDocwalks the entire document right afterupdateView.
Combined with the identical scan indispatchTransaction, this doubles the work whenever the consumer sets a new value. For large docs this can dominate the cost of updating.Consider re-using the memoisation/debounce strategy discussed in the earlier review (or parse metadata only once after the last batched update).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (8)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx(3 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts(2 hunks)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx(8 hunks)src/components/text-editor/text-editor.tsx(4 hunks)src/components/text-editor/text-editor.types.ts(2 hunks)src/components/text-editor/utils/metadata-utils.spec.ts(1 hunks)src/components/text-editor/utils/metadata-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`**/*.{ts,tsx}`: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
**/*.{ts,tsx}: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
src/components/text-editor/text-editor.types.tssrc/components/text-editor/prosemirror-adapter/plugins/link-plugin.tssrc/components/text-editor/prosemirror-adapter/plugins/image/inserter.tssrc/components/text-editor/utils/metadata-utils.spec.tssrc/components/text-editor/utils/metadata-utils.tssrc/components/text-editor/text-editor.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`**/*.tsx`: Our `.tsx` files are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from the `render` method, they will sometime...
**/*.tsx: Our.tsxfiles are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from therendermethod, they will sometimes wrap them in an array literal. In these cases, rather than recommending they addkeyproperties to the elements, recommend removing the hardcoded array literal. Recommend wrapping the elements in StencilJS's special<Host>element.
src/components/text-editor/text-editor.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`**/*.{tsx,scss}`: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
**/*.{tsx,scss}: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
src/components/text-editor/text-editor.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`src/components/**/examples/**/*.{ts,tsx}`: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is p...
src/components/**/examples/**/*.{ts,tsx}: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is publically exported by lime-elements, the import should be made from@limetech/lime-elements. If they import something from another file inside theexamplefolder, the import should use a relative path.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
🧠 Learnings (1)
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
Learnt from: FredrikWallstrom
PR: Lundalogik/lime-elements#3464
File: src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts:58-67
Timestamp: 2025-03-04T14:44:40.706Z
Learning: In the image inserter plugin for the text editor's ProseMirror adapter, only standard image nodes (where node.type.name === 'image') are intentionally handled, as custom image-like nodes are not within the current scope.
🧬 Code Graph Analysis (5)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
src/components/text-editor/text-editor.types.ts (1)
LinkInfo(125-135)
src/components/text-editor/utils/metadata-utils.ts (1)
src/components/text-editor/text-editor.types.ts (3)
MetadataInfo(181-191)ImageInfo(106-120)LinkInfo(125-135)
src/components/text-editor/text-editor.tsx (2)
src/interface.ts (1)
EventEmitter(16-16)src/components/text-editor/text-editor.types.ts (2)
ImageInfo(106-120)MetadataInfo(181-191)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (1)
src/components/text-editor/text-editor.types.ts (2)
MetadataInfo(181-191)ImageInfo(106-120)
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (5)
src/components/text-editor/text-editor.types.ts (2)
MetadataInfo(181-191)LinkInfo(125-135)src/interface.ts (1)
EventEmitter(16-16)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
createImageInserterPlugin(12-28)src/components/text-editor/utils/metadata-utils.ts (2)
getMetadataFromDoc(13-38)hasMetadataChanged(48-70)src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (1)
imageCache(7-7)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Build
- GitHub Check: Test
- GitHub Check: Lint
🔇 Additional comments (10)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
7-7: Adding LinkInfo import aligns with metadata structure standardization.The import of LinkInfo from a shared location rather than using a local type definition is a good step toward standardizing the metadata handling across the editor ecosystem.
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (2)
1-5: Good cleanup: removing unnecessary state managementThe plugin has been simplified by removing the image tracking state and removal callback. This is a positive architectural change that moves the responsibility for tracking image changes to the centralized metadata system.
12-14: Clean function signature aligns with architectural changesRemoving the
imageRemovedCallbackparameter simplifies the API and correctly reflects that image removal detection has been moved to the centralized metadata tracking system.src/components/text-editor/text-editor.tsx (6)
11-11: Good practice: Import the new type that's being usedCorrectly importing the
MetadataInfotype for the new event emitter.
190-192: Proper deprecation notice for the imageRemoved eventGood practice to include a clear deprecation notice with guidance on using the new
metadataChangeevent instead.
196-204: New metadata event enables unified tracking of images and linksThe new
metadataChangeevent is a good addition that centralizes metadata tracking for both images and links, which is a cleaner architectural approach than separate events.
283-283: Wiring up the new event handler to the componentCorrectly connecting the
onMetadataChangeevent from the ProseMirror adapter to the local handler.
346-349: Clean handler implementation for the new eventThe
handleMetadataChangemethod follows the same pattern as the other event handlers, ensuring consistency in the codebase.
353-353: Properly suppressing linting warnings for deprecated codeGood practice to disable the deprecation warning for code that's intentionally using deprecated functionality for backward compatibility.
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (1)
499-505: Emit ofmetadataChangehappens whilesuppressChangeEventis active
updateViewsetssuppressChangeEvent = trueto silence normalchangeevents, butmetadataEmitterwill still emitmetadataChange.
Double-check that callers expect a metadata event when the value is programmatically replaced; if not, guard the emitter the same way you guard thechangeevent.- this.metadataEmitter(metadata); + if (!this.suppressChangeEvent) { + this.metadataEmitter(metadata); + }
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
125-133:⚠️ Potential issueNull-safety bug – destructuring may throw when no link is found
getLinkDataAtPositioncan returnnull, yet the result is destructured unconditionally:const { href } = getLinkDataAtPosition(view, event); // ❌ TypeError if nullGuard against the
nullcase to prevent runtime crashes:-const { href } = getLinkDataAtPosition(view, event); -if (href) { +const linkData = getLinkDataAtPosition(view, event); +if (linkData?.href) { + const { href } = linkData;src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
172-217: 🧹 Nitpick (assertive)Minor: distinguish non-image files in paste handler
processPasteEventreturnsfiles.length > 0, preventing default paste behaviour even when all clipboard files are non-images. Consider returningfalseunless an actual image was handled, e.g.:- return files.length > 0; + return files.some((f) => f.type.startsWith('image/'));
♻️ Duplicate comments (6)
src/components/text-editor/text-editor.types.ts (2)
122-135: Provide a concrete usage example in the JSDoc forLinkInfoA brief code snippet (similar to the style used for
ImageInfo) makes the interface easier to grasp at a glance./** * @example * ```ts * const link: LinkInfo = { href: 'https://lime-crm.com', text: 'Lime CRM' }; * ``` */
181-191: Exposeimagesandlinksas readonly collectionsThe earlier review already highlighted that mutating these arrays from outside may break the diff-logic relying on immutability. Switching to
ReadonlyArray<T>is a lightweight guard.-export interface MetadataInfo { - images: ImageInfo[]; - links: LinkInfo[]; +export interface MetadataInfo { + readonly images: ReadonlyArray<ImageInfo>; + readonly links: ReadonlyArray<LinkInfo>; }src/components/text-editor/utils/metadata-utils.ts (1)
90-97: Harden key construction for missing link attributes
hrefortextcould beundefined(malformed mark or programmatic manipulation), producing the literal string"undefined"in the key. Apply the same null-protection you added for images.- const key = `${link.href}|${link.text}`; + const key = `${link.href ?? ''}|${link.text ?? ''}`;src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
136-141: Pass the wholeLinkInfoobject toopenLinkMenufor future-proofingOnly half of the earlier recommendation was implemented: the event type is now
LinkInfo, but the helper still takes two strings, leading to extra parameters next time a field is added.-const openLinkMenu = (view: EditorView, href: string, text: string) => { - const event = new CustomEvent<LinkInfo>('open-editor-link-menu', { - detail: { href: href, text: text }, +const openLinkMenu = (view: EditorView, link: LinkInfo) => { + const event = new CustomEvent<LinkInfo>('open-editor-link-menu', { + detail: link, bubbles: true, composed: true, }); view.dom.dispatchEvent(event); };Call-sites then become
openLinkMenu(view, { href, text }).src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (1)
147-155: Implementation still missing forremoveImageThe method throws a Not implemented error and therefore always logs a
failure. Consumers copying this example might overlook the placeholder.If the intent is to leave removal as an exercise, add a clearer TODO comment.
Otherwise supply at least a stub that succeeds (or rejects) based on an
injected service.Tagged as duplicate of a previous review comment on the same issue.
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (1)
471-473: Full-document metadata extraction runs on every programmatic update
getMetadataFromDocis invoked after everyupdateView, repeating the same
tree walk already performed insidehandleTransaction.
For large documents this can become expensive.Previous feedback suggested debouncing, memoising, or extracting metadata
incrementally from the transaction steps.(Keeping as a reminder; see earlier discussion.)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (8)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx(3 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts(2 hunks)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx(8 hunks)src/components/text-editor/text-editor.tsx(4 hunks)src/components/text-editor/text-editor.types.ts(2 hunks)src/components/text-editor/utils/metadata-utils.spec.ts(1 hunks)src/components/text-editor/utils/metadata-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`**/*.{ts,tsx}`: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
**/*.{ts,tsx}: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.tssrc/components/text-editor/text-editor.types.tssrc/components/text-editor/utils/metadata-utils.spec.tssrc/components/text-editor/prosemirror-adapter/plugins/image/inserter.tssrc/components/text-editor/text-editor.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/utils/metadata-utils.tssrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`**/*.tsx`: Our `.tsx` files are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from the `render` method, they will sometime...
**/*.tsx: Our.tsxfiles are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from therendermethod, they will sometimes wrap them in an array literal. In these cases, rather than recommending they addkeyproperties to the elements, recommend removing the hardcoded array literal. Recommend wrapping the elements in StencilJS's special<Host>element.
src/components/text-editor/text-editor.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`**/*.{tsx,scss}`: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
**/*.{tsx,scss}: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
src/components/text-editor/text-editor.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`src/components/**/examples/**/*.{ts,tsx}`: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is p...
src/components/**/examples/**/*.{ts,tsx}: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is publically exported by lime-elements, the import should be made from@limetech/lime-elements. If they import something from another file inside theexamplefolder, the import should use a relative path.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
🧠 Learnings (1)
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
Learnt from: FredrikWallstrom
PR: Lundalogik/lime-elements#3464
File: src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts:58-67
Timestamp: 2025-03-04T14:44:40.706Z
Learning: In the image inserter plugin for the text editor's ProseMirror adapter, only standard image nodes (where node.type.name === 'image') are intentionally handled, as custom image-like nodes are not within the current scope.
🧬 Code Graph Analysis (4)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
src/components/text-editor/text-editor.types.ts (1)
LinkInfo(125-135)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (1)
src/components/text-editor/text-editor.types.ts (2)
MetadataInfo(181-191)ImageInfo(106-120)
src/components/text-editor/utils/metadata-utils.ts (1)
src/components/text-editor/text-editor.types.ts (3)
MetadataInfo(181-191)ImageInfo(106-120)LinkInfo(125-135)
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (5)
src/components/text-editor/text-editor.types.ts (2)
MetadataInfo(181-191)LinkInfo(125-135)src/interface.ts (1)
EventEmitter(16-16)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
createImageInserterPlugin(12-28)src/components/text-editor/utils/metadata-utils.ts (2)
getMetadataFromDoc(13-38)hasMetadataChanged(48-70)src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (1)
imageCache(7-7)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Lint
🔇 Additional comments (1)
src/components/text-editor/utils/metadata-utils.spec.ts (1)
1-306: Well-structured and comprehensive test suite for metadata utilities.The test file thoroughly covers both utility functions with tests for various scenarios:
getMetadataFromDoc: Tests extraction from documents with only images, only links, and mixed contenthasMetadataChanged: Tests differences in count, content, handling duplicates, order-independence, and identical metadata casesThe test schema and helpers are well-designed to simulate the editor's document structure.
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
Outdated
Show resolved
Hide resolved
| * @alpha | ||
| */ | ||
| @Event() | ||
| private readonly metadataChange: EventEmitter<MetadataInfo>; |
There was a problem hiding this comment.
Should events be private? I don't understand why all Events are private in this file
There was a problem hiding this comment.
Good question, I was really confused about if they should be private or public. It looks like we have a wild mix across the board in lime-elements repository.
There was a problem hiding this comment.
I think the discrepancy stems from me and @jgroth having different opinions on this. My reasoning has been that the event property should be private, because it holds the EventEmitter, and I don't think there's any reason that code outside the component should be able to call the event emitter.
But the private "tag" is just a compiler hint. It doesn't have any impact at all once the code has been compiled to javascript, so it doesn't really matter. It's fine if you want to go with public instead.
Either way, the event is still included in the docs. The docs currently show a private label, but unless I'm completely mistaken, that's only because the docblock includes the @private tag.
There was a problem hiding this comment.
For Stencil components, public/private properties has no impact on the code either 😄 The component instance is completely hidden and a consumer can never access it unless it's somehow exposed manually, and I don't think that's a very common pattern that we have ever done. The consumer typically only has access to the DOM element, where properties decorated with @Prop will be added
There was a problem hiding this comment.
The Stencil docs does not seem to use public/private for anything, not for @Prop or @State either, which will make everything implicitly public
| /** | ||
| * Collection of link elements found in the document | ||
| */ | ||
| links: LinkInfo[]; |
There was a problem hiding this comment.
I think we can use the Link interface from here. We don't need a new interface LinkInfo
There was a problem hiding this comment.
Good point, should we mix the Links in limel-text-editor with that global shared Link interface?
I can see that we do also have a global shared Image interface. However, it is missing some some properties if we compare to ImageInfo interface.
To me it feels a little bit inconsistent if we use the global shared interface for links but not for images 🤔
There was a problem hiding this comment.
Yea we can extend ImageInfo like this. It is good to include the parameter alt for accessibility purpose as well, which is already included in the Image interface
export interface ImageInfo extends Image {
There was a problem hiding this comment.
For clear distinction, isn't it good to keep Editor interfaces' separated from "global shared" interfaces'?
I pushed a fixup to make this distinction more clear: 8644b02
There was a problem hiding this comment.
I agree. For the purposes of having everything related to the text-editor clearly distinct from other shared interfaces we've added our own interfaces, usually prefaced by Editor.
My suggestion would also be to keep the clear distinction.
| `Failed to delete image ${imageInfo.fileInfoId}`, | ||
| error, | ||
| ); | ||
| console.error(`Failed to remove image ${image.fileInfoId}`, error); |
There was a problem hiding this comment.
It would have been better to have a log on line 151 instead of throwing an error.
console.log(Removing image: ${image.fileInfoId});
However it is not added by this PR though.
| return oldMetadata.images.filter( | ||
| (oldImage) => | ||
| !newMetadata.images.some( | ||
| (newImage) => newImage.fileInfoId === oldImage.fileInfoId, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Maybe better to avoid nested loop ? And using a Set for faster lookups, improving performance when comparing
| return oldMetadata.images.filter( | |
| (oldImage) => | |
| !newMetadata.images.some( | |
| (newImage) => newImage.fileInfoId === oldImage.fileInfoId, | |
| ), | |
| ); | |
| const newImageIds = new Set( | |
| newMetadata.images.map((image) => image.fileInfoId), | |
| ); | |
| return oldMetadata.images.filter( | |
| (image) => !newImageIds.has(image.fileInfoId), | |
| ); |
| /** | ||
| * Collection of link elements found in the document | ||
| */ | ||
| links: LinkInfo[]; |
There was a problem hiding this comment.
Yea we can extend ImageInfo like this. It is good to include the parameter alt for accessibility purpose as well, which is already included in the Image interface
export interface ImageInfo extends Image {
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
136-143: 🧹 Nitpick (assertive)Event payload construction can be simplified & made future-proof
You've updated the event type to use
EditorLink, but the event construction can be further simplified by passing the whole link object instead of separate properties.-const openLinkMenu = (view: EditorView, href: string, text: string) => { - const event = new CustomEvent<EditorLink>('open-editor-link-menu', { - detail: { href: href, text: text }, +const openLinkMenu = (view: EditorView, link: EditorLink) => { + const event = new CustomEvent<EditorLink>('open-editor-link-menu', { + detail: link, bubbles: true, composed: true, }); view.dom.dispatchEvent(event); }Call-site update (line 169):
-openLinkMenu(view, href, text); +openLinkMenu(view, { href, text });src/components/text-editor/text-editor.types.ts (1)
175-191: 🧹 Nitpick (assertive)Consider immutability for
EditorMetadatacollectionsThe new interface structure is clear and well-documented, but the arrays could be made immutable to prevent accidental mutations.
export interface EditorMetadata { /** * Collection of image elements found in the document */ - images: EditorImage[]; + readonly images: ReadonlyArray<EditorImage>; /** * Collection of link elements found in the document */ - links: EditorLink[]; + readonly links: ReadonlyArray<EditorLink>; }This keeps metadata snapshots immutable and prevents accidental side-effects that could bypass internal diffing logic.
src/components/text-editor/text-editor.tsx (2)
196-204: 🧹 Nitpick (assertive)Add
readonlyto the new event emitter for consistencyEvery other
@Event()in this component is declared withprivate readonly. Addingreadonlyhere would maintain consistency.- private readonly metadataChange: EventEmitter<EditorMetadata>; + private readonly metadataChange: EventEmitter<EditorMetadata>;
346-349: 🧹 Nitpick (assertive)Consider bubbling the original event instead of stopping & re-emitting
Calling
event.stopPropagation()prevents the original event from reaching parent components, and then you re-emit a new event with the same name.If there's no need to manipulate the event detail, you could simply let the original event bubble out and remove this listener altogether, reducing work and avoiding double events.
- private handleMetadataChange = (event: CustomEvent<EditorMetadata>) => { - event.stopPropagation(); - this.metadataChange.emit(event.detail); - };Note: Keep the current approach if you intend to manipulate the payload or need to maintain strict encapsulation of the adapter.
src/components/text-editor/utils/metadata-utils.ts (1)
23-32: 🛠️ Refactor suggestionDuplicates in link extraction can trigger false-positive change events
getMetadataFromDocpushes alinksentry for every text node that carries alinkmark.
When a single logical link spans multiple text nodes (e.g. after bold/italic splits), you will end up with several identical{ href, text }objects.
BecausehasMetadataChangedlater relies on frequency maps, these duplicates will inflate the counts and report a change even when nothing has actually changed from the user’s perspective.Consider deduplicating on the fly:
+const seenLinks = new Set<string>(); … if (mark.type.name === 'link' && mark.attrs) { - metadata.links.push({ href: mark.attrs.href, text: node.text }); + const key = `${mark.attrs.href}|${node.text}`; + if (!seenLinks.has(key)) { + metadata.links.push({ href: mark.attrs.href, text: node.text }); + seenLinks.add(key); + } }This keeps the comparison stable regardless of ProseMirror’s internal node splitting/merging.
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (2)
471-473: Full-document scans on every update still presentAs flagged in a previous review,
getMetadataFromDoctraverses the entire document both inupdateViewand for every dispatched transaction. On large docs this can dominate typing latency.If incremental extraction is too involved, a cheap win is to memoise the
docreference and skip extraction when it hasn’t changed, or debounce the call during rapid typing.(Re-raising for visibility; feel free to ignore if performance is acceptable for your usage.)
Also applies to: 491-493
519-521: 🧹 Nitpick (assertive)Silence the deprecation warning when emitting
imageRemoved
imageRemovedis deprecated but still emitted for backwards-compatibility. Add the standard ESLint suppression comment to keep CI clean:imageCache.delete(image.fileInfoId); -this.imageRemoved.emit(image); +// eslint-disable-next-line sonarjs/deprecation +this.imageRemoved.emit(image);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (7)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx(3 hunks)src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts(2 hunks)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx(8 hunks)src/components/text-editor/text-editor.tsx(4 hunks)src/components/text-editor/text-editor.types.ts(3 hunks)src/components/text-editor/utils/metadata-utils.spec.ts(1 hunks)src/components/text-editor/utils/metadata-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`**/*.{ts,tsx}`: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
**/*.{ts,tsx}: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
src/components/text-editor/text-editor.types.tssrc/components/text-editor/utils/metadata-utils.spec.tssrc/components/text-editor/prosemirror-adapter/plugins/link-plugin.tssrc/components/text-editor/text-editor.tsxsrc/components/text-editor/utils/metadata-utils.tssrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`**/*.tsx`: Our `.tsx` files are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from the `render` method, they will sometime...
**/*.tsx: Our.tsxfiles are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from therendermethod, they will sometimes wrap them in an array literal. In these cases, rather than recommending they addkeyproperties to the elements, recommend removing the hardcoded array literal. Recommend wrapping the elements in StencilJS's special<Host>element.
src/components/text-editor/text-editor.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`**/*.{tsx,scss}`: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
**/*.{tsx,scss}: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
src/components/text-editor/text-editor.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`src/components/**/examples/**/*.{ts,tsx}`: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is p...
src/components/**/examples/**/*.{ts,tsx}: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is publically exported by lime-elements, the import should be made from@limetech/lime-elements. If they import something from another file inside theexamplefolder, the import should use a relative path.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
🧬 Code Graph Analysis (5)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
src/components/text-editor/text-editor.types.ts (1)
EditorLink(125-135)
src/components/text-editor/text-editor.tsx (2)
src/interface.ts (1)
EventEmitter(16-16)src/components/text-editor/text-editor.types.ts (2)
EditorImage(106-120)EditorMetadata(181-191)
src/components/text-editor/utils/metadata-utils.ts (1)
src/components/text-editor/text-editor.types.ts (3)
EditorMetadata(181-191)EditorImage(106-120)EditorLink(125-135)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (1)
src/components/text-editor/text-editor.types.ts (2)
EditorMetadata(181-191)EditorImage(106-120)
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (5)
src/components/text-editor/text-editor.types.ts (3)
EditorMetadata(181-191)EditorImage(106-120)EditorLink(125-135)src/interface.ts (1)
EventEmitter(16-16)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
createImageInserterPlugin(12-28)src/components/text-editor/utils/metadata-utils.ts (2)
getMetadataFromDoc(13-38)hasMetadataChanged(48-70)src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (1)
imageCache(7-7)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Build
🔇 Additional comments (12)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
7-7: Import addition looks goodThe import of
EditorLinkis appropriate for the updated event type.src/components/text-editor/text-editor.types.ts (1)
106-120: Renaming interfaces for consistencyGood choice renaming
ImageInfotoEditorImageto maintain a consistent naming convention across related types.src/components/text-editor/utils/metadata-utils.spec.ts (5)
1-36: Well-structured test setupThe test setup is clean and comprehensive, correctly defining a custom ProseMirror schema with the necessary nodes and marks for testing.
37-74: Good test coverage for image extractionThe test thoroughly validates that image metadata is correctly extracted from the document, checking all relevant properties.
76-122: Good test coverage for link extractionThe test thoroughly validates that link metadata is correctly extracted from the document, checking both href and text content.
124-167: Comprehensive test for complex document handlingGood job testing a mixed document containing both images and links to ensure the extraction works correctly in realistic scenarios.
169-305: Thorough metadata comparison testsThe tests for
hasMetadataChangedcover all key scenarios: differing counts, content changes, duplicates, order changes, and identical metadata. This provides robust validation of the comparison logic.src/components/text-editor/text-editor.tsx (4)
10-12: Import updates look goodThe imports have been correctly updated to use the renamed types.
190-195: Good deprecation approachProperly deprecating the
imageRemovedevent while maintaining backward compatibility, with a clear migration path to the newmetadataChangeevent.
283-283: Event binding looks goodCorrectly binding to the new
metadataChangeevent from the ProseMirror adapter.
351-355: Good handling of deprecated eventThe ESLint disable comment is appropriate for suppressing deprecation warnings in the handler for the deprecated event.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (1)
116-131: Nice and clear migration tometadataChange🎉The handler correctly derives removed images and delegates cleanup before updating the local cache.
Good use ofSetfor O(n) look-ups, and the order of operations avoids race conditions. Nothing to add here.
| doc.descendants((node) => { | ||
| if (node.type.name === 'image' && node.attrs) { | ||
| metadata.images.push({ | ||
| src: node.attrs.src, | ||
| state: node.attrs.state, | ||
| fileInfoId: node.attrs.fileInfoId, | ||
| }); | ||
| } else if (node.isText && node.marks?.length) { | ||
| node.marks.forEach((mark) => { | ||
| if (mark.type.name === 'link' && mark.attrs) { | ||
| metadata.links.push({ | ||
| href: mark.attrs.href, | ||
| text: node.text, | ||
| }); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
Can we refactor this a bit for readability like this?
| doc.descendants((node) => { | |
| if (node.type.name === 'image' && node.attrs) { | |
| metadata.images.push({ | |
| src: node.attrs.src, | |
| state: node.attrs.state, | |
| fileInfoId: node.attrs.fileInfoId, | |
| }); | |
| } else if (node.isText && node.marks?.length) { | |
| node.marks.forEach((mark) => { | |
| if (mark.type.name === 'link' && mark.attrs) { | |
| metadata.links.push({ | |
| href: mark.attrs.href, | |
| text: node.text, | |
| }); | |
| } | |
| }); | |
| } | |
| return true; | |
| doc.descendants((node) => { | |
| if (isImageNode(node)) { | |
| metadata.images.push(extractImageMetadata(node)); | |
| } else if (isTextNodeWithMarks(node)) { | |
| extractLinkMetadata(node).forEach((link) => metadata.links.push(link)); | |
| } | |
| return true; | |
| }); | |
| function isImageNode(node: Node): boolean { | |
| return node.type.name === 'image' && !!node.attrs; | |
| } | |
| function extractImageMetadata(node: Node): ImageInfo { | |
| return { | |
| src: node.attrs.src, | |
| state: node.attrs.state, | |
| fileInfoId: node.attrs.fileInfoId, | |
| alt: node.attrs.alt, | |
| }; | |
| } | |
| function isTextNodeWithMarks(node: Node): boolean { | |
| return node.isText && node.marks?.length > 0; | |
| } | |
| function extractLinkMetadata(node: Node): LinkInfo[] { | |
| return node.marks | |
| .filter((mark) => mark.type.name === 'link' && mark.attrs) | |
| .map((mark) => ({ | |
| href: mark.attrs.href, | |
| text: node.text, | |
| })); | |
| } |
| if ( | ||
| oldMetadata.images.length !== newMetadata.images.length || | ||
| oldMetadata.links.length !== newMetadata.links.length | ||
| ) { | ||
| return true; | ||
| } | ||
|
|
||
| const oldLinkCounts = getLinkFrequencyMap(oldMetadata.links); | ||
| const newLinkCounts = getLinkFrequencyMap(newMetadata.links); | ||
|
|
||
| if (!areFrequencyMapsEqual(oldLinkCounts, newLinkCounts)) { | ||
| return true; | ||
| } | ||
|
|
||
| const oldImageCounts = getImageFrequencyMap(oldMetadata.images); | ||
| const newImageCounts = getImageFrequencyMap(newMetadata.images); | ||
|
|
||
| return !areFrequencyMapsEqual(oldImageCounts, newImageCounts); | ||
| } |
There was a problem hiding this comment.
If I understand correctly here this function checks if the meta data is changed or not and we have 3 conditions.
hasDifferentLengths , hasDifferentLinks and hasDifferentImages
| if ( | |
| oldMetadata.images.length !== newMetadata.images.length || | |
| oldMetadata.links.length !== newMetadata.links.length | |
| ) { | |
| return true; | |
| } | |
| const oldLinkCounts = getLinkFrequencyMap(oldMetadata.links); | |
| const newLinkCounts = getLinkFrequencyMap(newMetadata.links); | |
| if (!areFrequencyMapsEqual(oldLinkCounts, newLinkCounts)) { | |
| return true; | |
| } | |
| const oldImageCounts = getImageFrequencyMap(oldMetadata.images); | |
| const newImageCounts = getImageFrequencyMap(newMetadata.images); | |
| return !areFrequencyMapsEqual(oldImageCounts, newImageCounts); | |
| } | |
| return ( | |
| hasDifferentLengths(oldMetadata, newMetadata) || | |
| hasDifferentLinks(oldMetadata.links, newMetadata.links) || | |
| hasDifferentImages(oldMetadata.images, newMetadata.images) | |
| ); | |
| function hasDifferentLengths( | |
| oldMetadata: MetadataInfo, | |
| newMetadata: MetadataInfo, | |
| ): boolean { | |
| return ( | |
| oldMetadata.images.length !== newMetadata.images.length || | |
| oldMetadata.links.length !== newMetadata.links.length | |
| ); | |
| } | |
| function hasDifferentLinks( | |
| oldLinks: LinkInfo[], | |
| newLinks: LinkInfo[], | |
| ): boolean { | |
| const oldLinkCounts = getLinkFrequencyMap(oldLinks); | |
| const newLinkCounts = getLinkFrequencyMap(newLinks); | |
| return !areFrequencyMapsEqual(oldLinkCounts, newLinkCounts); | |
| } | |
| function hasDifferentImages( | |
| oldImages: ImageInfo[], | |
| newImages: ImageInfo[], | |
| ): boolean { | |
| const oldImageCounts = getImageFrequencyMap(oldImages); | |
| const newImageCounts = getImageFrequencyMap(newImages); | |
| return !areFrequencyMapsEqual(oldImageCounts, newImageCounts); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/components/text-editor/utils/metadata-utils.ts (2)
16-26: 🧹 Nitpick (assertive)Avoid duplicate link entries in metadata.
Each text node with a link mark currently pushes its metadata, which can produce duplicate entries when a logical link spans multiple nodes. Consider deduplicating entries to keep the metadata arrays concise and stable:doc.descendants((node) => { if (isImageNode(node)) { metadata.images.push(extractImageMetadata(node)); } else if (isTextNodeWithMarks(node)) { - extractLinkMetadata(node).forEach((link) => - metadata.links.push(link), - ); + extractLinkMetadata(node).forEach((link) => { + const exists = metadata.links.some( + (l) => l.href === link.href && l.text === link.text, + ); + if (!exists) { + metadata.links.push(link); + } + }); } return true; });
105-133: 🧹 Nitpick (assertive)Guard against undefined in frequency-map keys.
Interpolating potentiallyundefinedproperties may produce the literal"undefined"and lead to key collisions. Adding nullish coalescing makes the maps more robust:function getImageFrequencyMap(images: EditorImage[]): Map<string, number> { const countMap = new Map<string, number>(); images.forEach((image) => { - const key = `${image.fileInfoId}|${image.state}|${image.src}`; + const key = `${image.fileInfoId ?? ''}|${image.state ?? ''}|${image.src ?? ''}`; countMap.set(key, (countMap.get(key) || 0) + 1); }); return countMap; } function getLinkFrequencyMap(links: EditorLink[]): Map<string, number> { const countMap = new Map<string, number>(); links.forEach((link) => { - const key = `${link.href}|${link.text}`; + const key = `${link.href ?? ''}|${link.text ?? ''}`; countMap.set(key, (countMap.get(key) || 0) + 1); }); return countMap; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/components/text-editor/utils/metadata-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
**/*.{ts,tsx}: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
src/components/text-editor/utils/metadata-utils.ts
🧬 Code Graph Analysis (1)
src/components/text-editor/utils/metadata-utils.ts (1)
src/components/text-editor/text-editor.types.ts (3)
EditorMetadata(181-191)EditorImage(106-120)EditorLink(125-135)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Build
🔇 Additional comments (4)
src/components/text-editor/utils/metadata-utils.ts (4)
1-2: Imports correctly use relative paths.
The imports from../text-editor.typesand the externalprosemirror-modelpackage are appropriate and adhere to the guideline requiring relative imports within this module.
56-73: Metadata change detection is clear and order-insensitive.
ThehasMetadataChangedfunction’s JSDoc is complete, and its logic—checking length differences followed by frequency-map comparisons—robustly handles reordering and duplicates.
75-103: Comparison helpers are concise and correct.
ThehasDifferentLengths,hasDifferentLinks, andhasDifferentImagesfunctions cleanly decompose the comparison steps. They correctly leverage frequency maps for order-insensitive equality.
135-153: Frequency-map equality comparison is efficient.
Your implementation correctly checks map sizes before iterating entries, allowing an early exit on mismatch. This is a clean and performant approach.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (1)
47-61:⚠️ Potential issueBroken state checks & legacy enum reference cause build failure
- Comparing to hard-coded
'foo'and'idontexist'looks accidental.ImageState.LOADINGis referenced butImageStateno longer exists.typeof node.attrs.state === 'EditorImageState'is an invalidtypeofcomparison (EditorImageStateis a type, not a JS value).- if (node.attrs.state === 'foo') { + if (node.attrs.state === 'failed') { ... - } else if (node.attrs.state === ImageState.LOADING) { + } else if (node.attrs.state === 'loading') { ... - if (node.attrs.state === 'idontexist') { + if (node.attrs.state === 'failed') { ... - } else if ( - node.attrs.state === 'loading' && - typeof node.attrs.state === 'EditorImageState' - ) { + } else if (node.attrs.state === 'loading') {These fixes align the checks with the new
EditorImageStatestring union and remove the invalidtypeofusage.
♻️ Duplicate comments (12)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
137-141: 🧹 Nitpick (assertive)Event-payload construction can be simplified & made future-proof
You can tighten this block a little and make
openLinkMenueasier to evolve by:
- Passing the whole
LinkInfoobject rather than two primitives – avoids parameter explosion when more link fields are added.- Using property-shorthand to reduce noise.
-const openLinkMenu = (view: EditorView, href: string, text: string) => { - const event = new CustomEvent<EditorLink>('open-editor-link-menu', { - detail: { href: href, text: text }, +const openLinkMenu = (view: EditorView, link: EditorLink) => { + const event = new CustomEvent<EditorLink>('open-editor-link-menu', { + detail: link, bubbles: true, composed: true, }); view.dom.dispatchEvent(event); }Call-site update (line 169):
- openLinkMenu(view, href, text); + openLinkMenu(view, { href, text });src/components/text-editor/text-editor.types.ts (2)
121-131: 🧹 Nitpick (assertive)New
EditorLinkinterface looks goodThe new interface is well-structured with meaningful JSDoc comments for each property.
Consider adding an example in the JSDoc showing how it's used, which helps consumers quickly understand intent.
/** + * Represents a link within the editor. + * + * @example + * ```ts + * const link: EditorLink = { + * href: 'https://example.com', + * text: 'Example Link' + * }; + * ``` * @alpha */
177-187: 🧹 Nitpick (assertive)Consider immutability for
EditorMetadatacollections
imagesandlinksare exposed as mutable arrays. If external code accidentally mutates them the internal diffing logic could be bypassed.export interface EditorMetadata { /** * Collection of image elements found in the document */ - images: EditorImage[]; + readonly images: ReadonlyArray<EditorImage>; /** * Collection of link elements found in the document */ - links: EditorLink[]; + readonly links: ReadonlyArray<EditorLink>; }This keeps metadata snapshots immutable and prevents accidental side-effects.
src/components/text-editor/text-editor.tsx (1)
346-349: 🧹 Nitpick (assertive)Event can bubble directly – re-emit is redundant
The listener stops the
metadataChangeevent coming from<limel-prosemirror-adapter>only to re-emit a new event with the exact same payload.
Unless you need to transformevent.detail, simply let the original event bubble and delete this listener to avoid an unnecessary dispatch and preserve the original composed path.src/components/text-editor/utils/metadata-utils.ts (4)
47-54: 🧹 Nitpick (assertive)Provide safe fallbacks when extracting link metadata
hrefortextcan be empty in edge cases. Add nullish coalescing to keep frequency maps stable..map((mark) => ({ - href: mark.attrs.href, - text: node.text, + href: mark.attrs.href ?? '', + text: node.text ?? '', }));
31-33: 🧹 Nitpick (assertive)Redundant
!!node.attrscheck
Node.attrsis always defined in ProseMirror, so the extra truthiness check is unnecessary.-function isImageNode(node: Node): boolean { - return node.type.name === 'image' && !!node.attrs; +function isImageNode(node: Node): boolean { + return node.type.name === 'image'; }
35-41: 🛠️ Refactor suggestionGuard against missing image attributes to avoid
undefinedin metadata keysIf a malformed document lacks any of the attributes, the current code will inject the literal string
"undefined"into frequency keys, causing accidental collisions.return { - src: node.attrs.src, - state: node.attrs.state, - fileInfoId: node.attrs.fileInfoId, + src: node.attrs.src ?? '', + state: (node.attrs.state ?? 'success') as EditorImage['state'], + fileInfoId: node.attrs.fileInfoId ?? '', };
111-115: 🛠️ Refactor suggestionDefensive key construction in frequency map
Same
undefined-collision issue as above. Coalesce each segment:-const key = `${image.fileInfoId}|${image.state}|${image.src}`; +const key = `${image.fileInfoId ?? ''}|${image.state ?? ''}|${image.src ?? ''}`;(Apply similar fix in
getLinkFrequencyMap.)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (4)
159-159: Reuse a shared EMPTY_METADATA constantWe previously suggested extracting the empty metadata object to a constant to avoid re-creating the same literal and to make intent clearer. The class still inlines the literal.
- private metadata: EditorMetadata = { images: [], links: [] }; + private metadata: EditorMetadata = EMPTY_METADATA;and at the end of the file:
const EMPTY_METADATA: EditorMetadata = { images: [], links: [] };This keeps allocations down and centralises the default value.
471-473: 🛠️ Refactor suggestionRedundant full-document scans on every update
getMetadataFromDocis invoked in bothupdateViewand every dispatched transaction, resulting in two complete tree walks per keystroke. This was flagged earlier; the latest code still performs the scans unchanged.Consider:
- Deriving metadata incrementally from
transaction.steps, or- Debouncing the extraction, or
- Memoising the last processed
doc.This will prevent metadata extraction from becoming the dominant cost while typing in large documents.
Also applies to: 491-493
499-505: 🧹 Nitpick (assertive)Inline comments would aid future maintainers
A short comment explaining why
hasMetadataChangedis followed byremoveImagesFromCache(order-insensitive comparison, cache clean-up, backwards-compat imageRemoved) would greatly improve readability for future maintainers.
507-522: 🧹 Nitpick (assertive)Suppress deprecation warning when emitting
imageRemoved
imageRemovedis deprecated but still emitted for backwards compatibility. Please add the ESLint suppression used elsewhere to avoid CI noise:- imageCache.delete(image.fileInfoId); - this.imageRemoved.emit(image); + imageCache.delete(image.fileInfoId); + // eslint-disable-next-line sonarjs/deprecation + this.imageRemoved.emit(image);Good job swapping the parameter order of
removeImagesFromCache; the API is now intuitive.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (9)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx(3 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts(4 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/node.ts(5 hunks)src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts(2 hunks)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx(8 hunks)src/components/text-editor/text-editor.tsx(4 hunks)src/components/text-editor/text-editor.types.ts(3 hunks)src/components/text-editor/utils/metadata-utils.spec.ts(1 hunks)src/components/text-editor/utils/metadata-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`**/*.{ts,tsx}`: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
**/*.{ts,tsx}: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.tssrc/components/text-editor/prosemirror-adapter/plugins/image/inserter.tssrc/components/text-editor/text-editor.types.tssrc/components/text-editor/utils/metadata-utils.spec.tssrc/components/text-editor/text-editor.tsxsrc/components/text-editor/utils/metadata-utils.tssrc/components/text-editor/prosemirror-adapter/plugins/image/node.tssrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`**/*.tsx`: Our `.tsx` files are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from the `render` method, they will sometime...
**/*.tsx: Our.tsxfiles are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from therendermethod, they will sometimes wrap them in an array literal. In these cases, rather than recommending they addkeyproperties to the elements, recommend removing the hardcoded array literal. Recommend wrapping the elements in StencilJS's special<Host>element.
src/components/text-editor/text-editor.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`**/*.{tsx,scss}`: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
**/*.{tsx,scss}: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
src/components/text-editor/text-editor.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
`src/components/**/examples/**/*.{ts,tsx}`: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is p...
src/components/**/examples/**/*.{ts,tsx}: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is publically exported by lime-elements, the import should be made from@limetech/lime-elements. If they import something from another file inside theexamplefolder, the import should use a relative path.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
🧠 Learnings (1)
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
Learnt from: FredrikWallstrom
PR: Lundalogik/lime-elements#3464
File: src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts:58-67
Timestamp: 2025-03-04T14:44:40.706Z
Learning: In the image inserter plugin for the text editor's ProseMirror adapter, only standard image nodes (where node.type.name === 'image') are intentionally handled, as custom image-like nodes are not within the current scope.
🧬 Code Graph Analysis (7)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
src/components/text-editor/text-editor.types.ts (1)
EditorLink(121-131)
src/components/text-editor/utils/metadata-utils.spec.ts (2)
src/components/text-editor/utils/metadata-utils.ts (2)
getMetadataFromDoc(13-29)hasMetadataChanged(64-73)src/components/text-editor/text-editor.types.ts (1)
EditorMetadata(177-187)
src/components/text-editor/text-editor.tsx (2)
src/interface.ts (1)
EventEmitter(16-16)src/components/text-editor/text-editor.types.ts (2)
EditorImage(102-116)EditorMetadata(177-187)
src/components/text-editor/utils/metadata-utils.ts (1)
src/components/text-editor/text-editor.types.ts (3)
EditorMetadata(177-187)EditorImage(102-116)EditorLink(121-131)
src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (1)
src/components/text-editor/text-editor.types.ts (1)
EditorImageState(97-97)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (1)
src/components/text-editor/text-editor.types.ts (2)
EditorMetadata(177-187)EditorImage(102-116)
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (5)
src/components/text-editor/text-editor.types.ts (3)
EditorMetadata(177-187)EditorImage(102-116)EditorLink(121-131)src/interface.ts (1)
EventEmitter(16-16)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
createImageInserterPlugin(12-28)src/components/text-editor/utils/metadata-utils.ts (2)
getMetadataFromDoc(13-29)hasMetadataChanged(64-73)src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (1)
imageCache(7-7)
🪛 Biome (1.9.4)
src/components/text-editor/prosemirror-adapter/plugins/image/node.ts
[error] 129-129: Invalid typeof comparison value: "editorimagestate" is not a valid type name
not a valid type name
(lint/suspicious/useValidTypeof)
🪛 ESLint
src/components/text-editor/prosemirror-adapter/plugins/image/node.ts
[error] 129-129: Invalid typeof comparison value.
(valid-typeof)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Build
🔇 Additional comments (19)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
7-7: Import change aligns with new centralized type systemGood addition of the
EditorLinkimport from the shared types file. This promotes consistency across the text editor components.src/components/text-editor/text-editor.types.ts (2)
97-97: Good move from enum to string union typeConverting from an enum to a string union type is a solid choice that improves type safety while maintaining readability and flexibility in your code.
102-116: Rename toEditorImageestablishes clear domain boundariesGood choice to rename the interface to follow the
Editor*naming pattern for consistency with other editor-specific types.src/components/text-editor/utils/metadata-utils.spec.ts (5)
1-31: Well-structured test setup with clear schema definitionThe test schema and helper functions provide a solid foundation for the test cases. The schema accurately represents the document structure with nodes and marks needed for testing metadata extraction.
37-74: Good test coverage for image extractionThe test properly verifies that images are correctly extracted from the document, checking all relevant properties like
src,fileInfoId, andstate.
76-122: Good test coverage for link extractionThe test ensures links are properly extracted with both
hrefand text content verified.
124-167: Comprehensive test for mixed content extractionThis test verifies that both images and links can be extracted from the same document, which is an important real-world scenario.
169-305: Thorough change detection test coverageThe tests cover all important scenarios for metadata change detection including:
- Count differences
- Content differences
- Duplicate handling
- Order independence
- Identity comparison
This comprehensive test suite will make the metadata utilities robust and reliable.
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (4)
1-5: Import simplification aligns with new metadata architectureRemoving state tracking imports and updating to use the new
EditorImageStatetype correctly aligns with the refactored metadata handling approach.
12-28: Plugin simplification removes redundant image trackingThe removal of
imageRemovedCallbackfrom the plugin signature and the associated state management is a good simplification. The metadata-based approach for tracking image removals is more centralized and consistent.
69-75: Consistent use of typed state constantSame pattern applied consistently here for the success state.
94-100: Consistent use of typed state constantSame pattern applied consistently here for the failed state.
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (1)
188-199: Verify whethermetadataChangeshould be publicThe new
metadataChangeevent is declaredprivate, meaning consumers of<limel-prosemirror-adapter>cannot subscribe to it—even though the PR description states that the purpose of the PR is “notifying about metadata changes”.If external components (e.g.
limel-text-editor) are expected to react, the event must be public:- @Event() - private metadataChange: EventEmitter<EditorMetadata>; + @Event() + public metadataChange!: EventEmitter<EditorMetadata>;Please confirm the intended visibility.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (6)
5-5: Type imports updated to match the new metadata APIThe imports have been correctly updated to use the new
EditorImageandEditorMetadatatypes that are part of the new metadata event system, replacing the previous image-specific types.Also applies to: 8-8
45-46: Private metadata property initialized appropriatelyThe metadata property is correctly initialized with empty arrays for links and images, following the structure defined in the
EditorMetadatainterface.
54-54: New metadata event listener addedThe
onMetadataChangeevent listener has been properly added to the text editor component, which will enable the component to react to changes in both image and link metadata.
115-130: Handler implementation for metadata changesThe implementation correctly:
- Identifies removed images by comparing old and new metadata
- Only removes images that were successfully uploaded (state === 'success')
- Updates the local metadata state after processing
This approach ensures proper cleanup of external resources when images are removed from the editor.
132-143: Efficient implementation of image removal detectionThis implementation uses a Set for efficient lookups when comparing image IDs, which is a performance improvement over nested loops, especially for documents with many images.
145-148: Image removal handler implementationThe
removeImagemethod now logs the removal rather than throwing an error, which is appropriate for an example component. In a real implementation, this would be where you'd add code to delete the image from external storage.
| const loadingState: EditorImageState = 'loading'; | ||
| const placeholderNode = schema.nodes.image.create({ | ||
| src: base64Data, | ||
| alt: fileInfo.filename, | ||
| fileInfoId: fileInfo.id, | ||
| state: ImageState.LOADING, | ||
| state: loadingState, | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Type safety improvement using string constants
Using a typed constant for the image state value adds clarity and type safety. This is a good practice when working with string literal types.
Consider extracting these state constants to shared constants if they're used in multiple places:
+// At the top of the file
+const IMAGE_STATE: Record<string, EditorImageState> = {
+ LOADING: 'loading',
+ SUCCESS: 'success',
+ FAILED: 'failed',
+};
// Then in the code
-const loadingState: EditorImageState = 'loading';
+const loadingState = IMAGE_STATE.LOADING;📝 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 loadingState: EditorImageState = 'loading'; | |
| const placeholderNode = schema.nodes.image.create({ | |
| src: base64Data, | |
| alt: fileInfo.filename, | |
| fileInfoId: fileInfo.id, | |
| state: ImageState.LOADING, | |
| state: loadingState, | |
| }); | |
| // At the top of the file (after imports) | |
| const IMAGE_STATE: Record<string, EditorImageState> = { | |
| LOADING: 'loading', | |
| SUCCESS: 'success', | |
| FAILED: 'failed', | |
| }; | |
| // …later in your code… | |
| const loadingState = IMAGE_STATE.LOADING; | |
| const placeholderNode = schema.nodes.image.create({ | |
| src: base64Data, | |
| alt: fileInfo.filename, | |
| fileInfoId: fileInfo.id, | |
| state: loadingState, | |
| }); |
🤖 Prompt for AI Agents (early access)
In src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts around lines 48 to 54, replace the inline string literal 'loading' used for the image state with a typed constant to improve type safety and clarity. Define this constant in a shared location if the same state values are used elsewhere, and import it here to maintain consistency and avoid duplication.
src/components/text-editor/prosemirror-adapter/plugins/image/node.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
♻️ Duplicate comments (4)
src/components/text-editor/utils/metadata-utils.spec.ts (4)
47-60: Prefer using theEditorImageStateenum over string literalsUsing the enum (
EditorImageState.SUCCESS) instead of the raw string"success"keeps the test data in sync with the production types and prevents accidental typos if the enum values ever change.attrs: { src: 'image1.jpg', fileInfoId: 'id1', - state: 'success', + state: EditorImageState.SUCCESS, }, }, { type: 'image', attrs: { src: 'image2.jpg', fileInfoId: 'id2', - state: 'success', + state: EditorImageState.SUCCESS, },
205-221: 🛠️ Refactor suggestionUse EditorImageState enum and fix formatting
Address both enum usage and formatting issues here as well.
test('hasMetadataChanged should return true when image content differs', () => { const oldMetadata: EditorMetadata = { images: [ - { src: 'img1.jpg', fileInfoId: 'id1', state: 'success' }, + { src: 'img1.jpg', fileInfoId: 'id1', state: EditorImageState.SUCCESS }, ], links: [], }; const newMetadata: EditorMetadata = { images: [ - { src: 'img2.jpg', fileInfoId: 'id1', state: 'success' }, + { src: 'img2.jpg', fileInfoId: 'id1', state: EditorImageState.SUCCESS }, ], links: [], }; expect(hasMetadataChanged(oldMetadata, newMetadata)).toBe(true); });🧰 Tools
🪛 ESLint
[error] 207-209: Replace
⏎············{·src:·'img1.jpg',·fileInfoId:·'id1',·state:·'success'·},⏎········with{·src:·'img1.jpg',·fileInfoId:·'id1',·state:·'success'·}(prettier/prettier)
[error] 214-216: Replace
⏎············{·src:·'img2.jpg',·fileInfoId:·'id1',·state:·'success'·},⏎········with{·src:·'img2.jpg',·fileInfoId:·'id1',·state:·'success'·}(prettier/prettier)
237-261: 🛠️ Refactor suggestionUse EditorImageState enum for consistency
For all image objects in this test case:
test('hasMetadataChanged should handle duplicate elements correctly', () => { const oldMetadata: EditorMetadata = { images: [ - { src: 'img1.jpg', fileInfoId: 'id1', state: 'success' }, - { src: 'img1.jpg', fileInfoId: 'id1', state: 'success' }, + { src: 'img1.jpg', fileInfoId: 'id1', state: EditorImageState.SUCCESS }, + { src: 'img1.jpg', fileInfoId: 'id1', state: EditorImageState.SUCCESS }, ], links: [ { href: 'https://example.com', text: 'Example' }, { href: 'https://example.com', text: 'Example' }, ], }; const newMetadata: EditorMetadata = { images: [ - { src: 'img1.jpg', fileInfoId: 'id1', state: 'success' }, - { src: 'img1.jpg', fileInfoId: 'id1', state: 'success' }, + { src: 'img1.jpg', fileInfoId: 'id1', state: EditorImageState.SUCCESS }, + { src: 'img1.jpg', fileInfoId: 'id1', state: EditorImageState.SUCCESS }, ], links: [ { href: 'https://example.com', text: 'Example' }, { href: 'https://example.com', text: 'Example' }, ], }; expect(hasMetadataChanged(oldMetadata, newMetadata)).toBe(false); });
263-287: 🛠️ Refactor suggestionUse EditorImageState enum in image metadata
Replace string literals with enum values:
test('hasMetadataChanged should ignore order changes', () => { const oldMetadata: EditorMetadata = { images: [ - { src: 'img1.jpg', fileInfoId: 'id1', state: 'success' }, - { src: 'img2.jpg', fileInfoId: 'id2', state: 'success' }, + { src: 'img1.jpg', fileInfoId: 'id1', state: EditorImageState.SUCCESS }, + { src: 'img2.jpg', fileInfoId: 'id2', state: EditorImageState.SUCCESS }, ], links: [ { href: 'https://example.com', text: 'Example' }, { href: 'https://test.com', text: 'Test' }, ], }; const newMetadata: EditorMetadata = { images: [ - { src: 'img2.jpg', fileInfoId: 'id2', state: 'success' }, - { src: 'img1.jpg', fileInfoId: 'id1', state: 'success' }, + { src: 'img2.jpg', fileInfoId: 'id2', state: EditorImageState.SUCCESS }, + { src: 'img1.jpg', fileInfoId: 'id1', state: EditorImageState.SUCCESS }, ], links: [ { href: 'https://test.com', text: 'Test' }, { href: 'https://example.com', text: 'Example' }, ], }; expect(hasMetadataChanged(oldMetadata, newMetadata)).toBe(false); });
🛑 Comments failed to post (8)
src/components/text-editor/prosemirror-adapter/plugins/image/view.ts (1)
177-184: 🧹 Nitpick (assertive)
Create
stateHandlersonce to avoid needless allocations
stateHandlersis recreated every timetransitionBetweenStates()runs.
Although the map is small, the method is called on every node-state update, so the extra object allocation is unnecessary. Moving the map to a module-levelconst(or making it a static class field) will save a bit of GC churn and express the intent that the mapping is immutable.- private transitionBetweenStates = () => { - this.cleanUpPreviousState(); - this.dom.className = `image-wrapper state-${this.node.attrs.state}`; - - const stateHandlers: Record<EditorImageState, () => void> = { - loading: this.createLoadingState, - success: this.createSuccessState, - failed: this.createFailedState, - }; +const STATE_HANDLERS: Record<EditorImageState, () => void> = { + loading: this.createLoadingState, + success: this.createSuccessState, + failed: this.createFailedState, +}; + + private transitionBetweenStates = () => { + this.cleanUpPreviousState(); + this.dom.className = `image-wrapper state-${this.node.attrs.state}`; - const state: EditorImageState = this.node.attrs.state; - stateHandlers[state]?.(); + const state: EditorImageState = this.node.attrs.state; + STATE_HANDLERS[state]?.(); };Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents (early access)
In src/components/text-editor/prosemirror-adapter/plugins/image/view.ts around lines 177 to 184, the stateHandlers object is recreated on every call to transitionBetweenStates(), causing unnecessary allocations. To fix this, move the stateHandlers map outside the method as a module-level constant or a static class field so it is created only once and reused, reducing garbage collection overhead and clarifying that the mapping is immutable.src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (3)
146-150: 🛠️ Refactor suggestion
Type-guard signature always returns
true; weakens safetyBecause the parameter is already typed as
EditorImageState, the function can never returnfalseat compile-time, defeating the purpose of a guard. Accept an unknown/string instead:-function isEditorImageState( - state: EditorImageState, -): state is EditorImageState { +function isEditorImageState(state: unknown): state is EditorImageState { return state === 'loading' || state === 'failed' || state === 'success'; }This lets TypeScript correctly narrow arbitrary values and prevents accidental misuse elsewhere.
🤖 Prompt for AI Agents (early access)
In src/components/text-editor/prosemirror-adapter/plugins/image/node.ts around lines 146 to 150, the isEditorImageState function currently takes a parameter already typed as EditorImageState, so it always returns true and does not act as a proper type guard. Change the parameter type to unknown or string to allow the function to check if the input is a valid EditorImageState value. This enables TypeScript to narrow arbitrary values correctly and improves type safety by preventing misuse.
122-127:
⚠️ Potential issue
toDOMreturns the wrong element for the"success"state – images disappear
isEditorImageState()currently returnstruefor"success", so the!isEditorImageState(...)check sends all valid states – including"success"– down the status-span path, meaning successful images render as a “loading” span instead of an<img>.- if (!isEditorImageState(node.attrs.state)) { - return getOrCreateImageElement(node.attrs.fileInfoId, node); - } - - return createStatusSpanForState(node.attrs.state, node, language); + if (node.attrs.state === 'loading' || node.attrs.state === 'failed') { + return createStatusSpanForState(node.attrs.state, node, language); + } + + // 'success' and any unknown/legacy value -> render the image + return getOrCreateImageElement(node.attrs.fileInfoId, node);Without this change, every saved document will “lose” its images in the rendered view & markdown export.
📝 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 (node.attrs.state === 'loading' || node.attrs.state === 'failed') { return createStatusSpanForState(node.attrs.state, node, language); } // 'success' and any unknown/legacy value -> render the image return getOrCreateImageElement(node.attrs.fileInfoId, node);🤖 Prompt for AI Agents (early access)
In src/components/text-editor/prosemirror-adapter/plugins/image/node.ts around lines 122 to 127, the toDOM function incorrectly returns a status span for the "success" state because isEditorImageState returns true for "success" and the code negates this check. Fix this by adjusting the condition to correctly distinguish the "success" state so that it returns the image element for "success" and only returns the status span for other editor image states. This ensures images render properly instead of disappearing.
46-58:
⚠️ Potential issueMarkdown serializer omits images in the
"success"stateSame root cause as in
toDOM: the guard treats"success"as a status state, so no HTML is emitted for normal images.- if (isEditorImageState(state)) { - const statusHTML = getStatusHTML(state, alt, language); - if (statusHTML) { - markdownSerializerState.write(statusHTML); - } - } else { + if (state === 'loading' || state === 'failed') { + const statusHTML = getStatusHTML(state, alt, language); + markdownSerializerState.write(statusHTML); + } else { // 'success' => normal <img> const imageHTML = getImageHTML(src, alt, width, maxWidth); markdownSerializerState.write(imageHTML); }This ensures exported/serialized markdown still contains valid
<img>tags for successfully uploaded images.📝 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.return (markdownSerializerState: MarkdownSerializerState, node: Node) => { const { state, alt, src, width, maxWidth } = node.attrs; if (state === 'loading' || state === 'failed') { const statusHTML = getStatusHTML(state, alt, language); markdownSerializerState.write(statusHTML); } else { // 'success' => normal <img> const imageHTML = getImageHTML(src, alt, width, maxWidth); markdownSerializerState.write(imageHTML); } };🤖 Prompt for AI Agents (early access)
In src/components/text-editor/prosemirror-adapter/plugins/image/node.ts around lines 46 to 58, the current check treats the "success" state as a status state and skips emitting the normal image HTML, causing images in the "success" state to be omitted in markdown serialization. Modify the condition to exclude the "success" state from the status check so that images with "success" state output the standard <img> HTML, ensuring they are included in the serialized markdown.src/components/text-editor/utils/metadata-utils.spec.ts (4)
1-3:
⚠️ Potential issueRemove the unused import
The
EditorImageStateimport is not being used in this file. You should remove it to keep the imports clean.import { Node, Schema } from 'prosemirror-model'; import { getMetadataFromDoc, hasMetadataChanged } from './metadata-utils'; -import { EditorImageState, EditorMetadata } from '../text-editor.types'; +import { EditorMetadata } from '../text-editor.types';📝 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.import { Node, Schema } from 'prosemirror-model'; import { getMetadataFromDoc, hasMetadataChanged } from './metadata-utils'; import { EditorMetadata } from '../text-editor.types';🧰 Tools
🪛 ESLint
[error] 3-3: Remove this unused import of 'EditorImageState'.
(sonarjs/unused-import)
[error] 3-3: 'EditorImageState' is defined but never used. Allowed unused vars must match /^h$/u.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents (early access)
In src/components/text-editor/utils/metadata-utils.spec.ts at lines 1 to 3, the import EditorImageState is not used anywhere in the file. Remove the EditorImageState import from the import statement to clean up unused imports.
150-154: 🛠️ Refactor suggestion
Use the EditorImageState enum here as well
For consistency and type safety, use the enum for the image state.
attrs: { src: 'image.jpg', fileInfoId: 'id123', - state: 'success', + state: EditorImageState.SUCCESS, },📝 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.attrs: { src: 'image.jpg', fileInfoId: 'id123', - state: 'success', + state: EditorImageState.SUCCESS, },🤖 Prompt for AI Agents (early access)
In src/components/text-editor/utils/metadata-utils.spec.ts around lines 150 to 154, the image state is currently set as a string literal 'success'. Replace this string with the corresponding value from the EditorImageState enum to ensure consistency and type safety. Import the enum if not already imported, and update the state property to use EditorImageState.success instead of the string.
289-305: 🛠️ Refactor suggestion
Use EditorImageState enum and fix formatting
Address both issues in this test case:
test('hasMetadataChanged should return false when metadata is identical', () => { const oldMetadata: EditorMetadata = { images: [ - { src: 'img1.jpg', fileInfoId: 'id1', state: 'success' }, + { src: 'img1.jpg', fileInfoId: 'id1', state: EditorImageState.SUCCESS }, ], links: [{ href: 'https://example.com', text: 'Example' }], }; const newMetadata: EditorMetadata = { images: [ - { src: 'img1.jpg', fileInfoId: 'id1', state: 'success' }, + { src: 'img1.jpg', fileInfoId: 'id1', state: EditorImageState.SUCCESS }, ], links: [{ href: 'https://example.com', text: 'Example' }], }; expect(hasMetadataChanged(oldMetadata, newMetadata)).toBe(false); });📝 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.test('hasMetadataChanged should return false when metadata is identical', () => { const oldMetadata: EditorMetadata = { images: [ { src: 'img1.jpg', fileInfoId: 'id1', state: EditorImageState.SUCCESS }, ], links: [{ href: 'https://example.com', text: 'Example' }], }; const newMetadata: EditorMetadata = { images: [ { src: 'img1.jpg', fileInfoId: 'id1', state: EditorImageState.SUCCESS }, ], links: [{ href: 'https://example.com', text: 'Example' }], }; expect(hasMetadataChanged(oldMetadata, newMetadata)).toBe(false); });🧰 Tools
🪛 ESLint
[error] 291-293: Replace
⏎············{·src:·'img1.jpg',·fileInfoId:·'id1',·state:·'success'·},⏎········with{·src:·'img1.jpg',·fileInfoId:·'id1',·state:·'success'·}(prettier/prettier)
[error] 298-300: Replace
⏎············{·src:·'img1.jpg',·fileInfoId:·'id1',·state:·'success'·},⏎········with{·src:·'img1.jpg',·fileInfoId:·'id1',·state:·'success'·}(prettier/prettier)
🤖 Prompt for AI Agents (early access)
In src/components/text-editor/utils/metadata-utils.spec.ts around lines 289 to 305, update the test to use the EditorImageState enum for the image state value instead of the string 'success'. Also, fix any formatting inconsistencies such as indentation or spacing to match the project's style guidelines.
169-186: 🛠️ Refactor suggestion
Use EditorImageState enum and fix formatting
Both the enum usage and formatting issues should be addressed in this test case.
test('hasMetadataChanged should return true when image counts differ', () => { const oldMetadata: EditorMetadata = { images: [ - { src: 'img1.jpg', fileInfoId: 'id1', state: 'success' }, - { src: 'img2.jpg', fileInfoId: 'id2', state: 'success' }, + { src: 'img1.jpg', fileInfoId: 'id1', state: EditorImageState.SUCCESS }, + { src: 'img2.jpg', fileInfoId: 'id2', state: EditorImageState.SUCCESS }, ], links: [], }; const newMetadata: EditorMetadata = { images: [ - { src: 'img1.jpg', fileInfoId: 'id1', state: 'success' }, + { src: 'img1.jpg', fileInfoId: 'id1', state: EditorImageState.SUCCESS }, ], links: [], }; expect(hasMetadataChanged(oldMetadata, newMetadata)).toBe(true); });📝 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.test('hasMetadataChanged should return true when image counts differ', () => { const oldMetadata: EditorMetadata = { images: [ { src: 'img1.jpg', fileInfoId: 'id1', state: EditorImageState.SUCCESS }, { src: 'img2.jpg', fileInfoId: 'id2', state: EditorImageState.SUCCESS }, ], links: [], }; const newMetadata: EditorMetadata = { images: [ { src: 'img1.jpg', fileInfoId: 'id1', state: EditorImageState.SUCCESS }, ], links: [], }; expect(hasMetadataChanged(oldMetadata, newMetadata)).toBe(true); });🧰 Tools
🪛 ESLint
[error] 179-181: Replace
⏎············{·src:·'img1.jpg',·fileInfoId:·'id1',·state:·'success'·},⏎········with{·src:·'img1.jpg',·fileInfoId:·'id1',·state:·'success'·}(prettier/prettier)
🤖 Prompt for AI Agents (early access)
In src/components/text-editor/utils/metadata-utils.spec.ts around lines 169 to 186, update the test to use the EditorImageState enum values instead of string literals for the image state property. Also, fix any formatting inconsistencies such as indentation or spacing to match the project's style guidelines.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/components/text-editor/text-editor.types.ts (2)
121-131: 🧹 Nitpick (assertive)Consider adding JSDoc example for EditorLink
Adding a usage example in the JSDoc would help consumers quickly understand how to create an EditorLink object.
/** + * Represents a link within the editor. + * + * @example + * ```ts + * const link: EditorLink = { + * href: 'https://example.com', + * text: 'Example Link' + * }; + * ``` * @alpha */
177-186: 🧹 Nitpick (assertive)Consider using immutable collections for EditorMetadata
The
imagesandlinksarrays are currently mutable, which could allow external code to accidentally modify them and bypass internal diffing logic.export interface EditorMetadata { /** * Collection of image elements found in the document */ - images: EditorImage[]; + readonly images: ReadonlyArray<EditorImage>; /** * Collection of link elements found in the document */ - links: EditorLink[]; + readonly links: ReadonlyArray<EditorLink>; }src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
49-54: 🧹 Nitpick (assertive)Consider using constants for image states
Using typed constants for image states would further improve type safety and maintainability.
+// At the top of the file +const IMAGE_STATE: Record<string, EditorImageState> = { + LOADING: 'loading', + SUCCESS: 'success', + FAILED: 'failed', +}; // Then in the calls to createImageNodeAttrs -'loading' +IMAGE_STATE.LOADING -'success' +IMAGE_STATE.SUCCESS -'failed' +IMAGE_STATE.FAILEDAlso applies to: 69-74, 93-98
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (1)
145-148: Image removal is still a stubThe method only logs the event. A real implementation (HTTP DELETE, cloud-storage SDK, etc.) is still needed or at least documented with TODOs for consumers of this example.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx(3 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts(5 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/node.ts(4 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/view.ts(2 hunks)src/components/text-editor/text-editor.types.ts(3 hunks)src/components/text-editor/utils/metadata-utils.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`**/*.{ts,tsx}`: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
**/*.{ts,tsx}: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.tssrc/components/text-editor/utils/metadata-utils.spec.tssrc/components/text-editor/prosemirror-adapter/plugins/image/view.tssrc/components/text-editor/text-editor.types.tssrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/plugins/image/node.ts
`**/*.tsx`: Our `.tsx` files are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from the `render` method, they will sometime...
**/*.tsx: Our.tsxfiles are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from therendermethod, they will sometimes wrap them in an array literal. In these cases, rather than recommending they addkeyproperties to the elements, recommend removing the hardcoded array literal. Recommend wrapping the elements in StencilJS's special<Host>element.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
`**/*.{tsx,scss}`: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
**/*.{tsx,scss}: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
`src/components/**/examples/**/*.{ts,tsx}`: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is p...
src/components/**/examples/**/*.{ts,tsx}: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is publically exported by lime-elements, the import should be made from@limetech/lime-elements. If they import something from another file inside theexamplefolder, the import should use a relative path.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
🧠 Learnings (1)
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
Learnt from: FredrikWallstrom
PR: Lundalogik/lime-elements#3464
File: src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts:58-67
Timestamp: 2025-03-04T14:44:40.706Z
Learning: In the image inserter plugin for the text editor's ProseMirror adapter, only standard image nodes (where node.type.name === 'image') are intentionally handled, as custom image-like nodes are not within the current scope.
🧬 Code Graph Analysis (4)
src/components/text-editor/utils/metadata-utils.spec.ts (2)
src/components/text-editor/utils/metadata-utils.ts (2)
getMetadataFromDoc(13-29)hasMetadataChanged(64-73)src/components/text-editor/text-editor.types.ts (1)
EditorMetadata(177-187)
src/components/text-editor/prosemirror-adapter/plugins/image/view.ts (1)
src/components/text-editor/text-editor.types.ts (1)
EditorImageState(97-97)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (1)
src/components/text-editor/text-editor.types.ts (2)
EditorMetadata(177-187)EditorImage(102-116)
src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (1)
src/components/text-editor/text-editor.types.ts (1)
EditorImageState(97-97)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Build
- GitHub Check: SonarCloud
🔇 Additional comments (12)
src/components/text-editor/prosemirror-adapter/plugins/image/view.ts (2)
177-185: Excellent refactoring to use a lookup pattern for state handlersThis refactoring is a nice improvement that replaces conditional statements with a lookup object. This approach is more maintainable, declarative, and easier to extend if new states are added in the future.
189-190: Consistent usage of string literals for statesThe update to use string literals for state comparisons aligns well with the transition from enum values to string union types.
src/components/text-editor/text-editor.types.ts (2)
97-97: Good use of string union type for image statesUsing a string union type provides better type safety while maintaining a clean, simple implementation.
102-116: EditorImage interface looks goodThe renamed interface with updated property types aligns well with the overall metadata tracking approach.
src/components/text-editor/utils/metadata-utils.spec.ts (5)
5-31: Good test schema setupThe schema setup nicely reflects the structure needed for testing, including the necessary attributes for images and links.
37-74: Well-structured test for image metadata extractionThis test thoroughly validates that image metadata is correctly extracted from the document, checking all relevant properties.
76-122: Comprehensive link metadata extraction testThe test effectively verifies that link metadata, including href and text content, is correctly extracted from the document.
124-167: Good test for complex document handlingTesting a document with both images and links ensures the metadata extraction works correctly in real-world scenarios.
169-295: Thorough tests for metadata change detectionThese tests cover all important scenarios for metadata comparison: count differences, content differences, duplicates, order changes, and identical metadata. The tests are well-structured and provide good coverage.
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (2)
13-15: Clean API simplification by removing the imageRemovedCallbackGood job removing the image removal callback parameter, which aligns with the PR's goal of centralizing metadata tracking through the new metadata event system.
110-121: Good extraction of createImageNodeAttrs helper functionExtracting this helper function improves code organization and reduces duplication across the three insertion methods.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (1)
45-46: 👍 Correct choice to keep a fresh metadata objectGood call keeping the initial metadata as a brand-new object literal to avoid the shared-reference pitfall we discussed earlier.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
Show resolved
Hide resolved
src/components/text-editor/prosemirror-adapter/plugins/image/node.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (1)
49-56:⚠️ Potential issueImages in
successstate are still dropped from Markdown/HTML outputThe branch conditioned on
isEditorImageState(state)also matches'success', so the serializer never reaches the fallback that emits the actual<img>tag.
This is the exact issue flagged in a previous review and it remains unresolved, meaning all successfully-loaded images disappear in exported content.- if (isEditorImageState(state)) { - const statusHTML = getStatusHTML(state, alt, language); - if (statusHTML) { - markdownSerializerState.write(statusHTML); - } - } else { + if (state === 'loading' || state === 'failed') { + const statusHTML = getStatusHTML(state, alt, language); + markdownSerializerState.write(statusHTML); + } else { // success and every unknown fallback const imageHTML = getImageHTML(src, alt, width, maxWidth); markdownSerializerState.write(imageHTML); }This restores correct serialization for images in
'success'state while keeping status placeholders for the other two states.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/components/text-editor/prosemirror-adapter/plugins/image/node.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
**/*.{ts,tsx}: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
src/components/text-editor/prosemirror-adapter/plugins/image/node.ts
🧬 Code Graph Analysis (1)
src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (1)
src/components/text-editor/text-editor.types.ts (1)
EditorImageState(97-97)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Lint
- GitHub Check: Build
- GitHub Check: Test
| function isEditorImageState(state: unknown): state is EditorImageState { | ||
| return state === 'loading' || state === 'failed' || state === 'success'; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Broaden (or rename) isEditorImageState to avoid logic-errors like the one above
Because the guard currently returns true for 'success', callers that simply want to know “is this a placeholder state?” end up with misleading results.
Either:
- Rename the function to something like
isValidImageStateand keep the current semantics, or - Narrow it to only
'loading' | 'failed'and update the few usages that genuinely need to include'success'.
This will reduce the risk of future regressions.
🤖 Prompt for AI Agents (early access)
In src/components/text-editor/prosemirror-adapter/plugins/image/node.ts around lines 150 to 152, the type guard function isEditorImageState currently returns true for 'loading', 'failed', and 'success', which can cause logic errors when callers expect only placeholder states. To fix this, either rename the function to isValidImageState to reflect that it includes 'success', or narrow the function to only return true for 'loading' and 'failed' and update all usages that require 'success' accordingly to prevent misleading results.
| fileInfoId: string, | ||
| node: Node, | ||
| ): HTMLImageElement { | ||
| let img = imageCache.get(fileInfoId); | ||
|
|
||
| if (img) { | ||
| updateImageElement(img, node); | ||
| } else { | ||
| img = createImageElement(node); | ||
| imageCache.set(fileInfoId, img); | ||
| } | ||
|
|
||
| return img; | ||
| } | ||
|
|
There was a problem hiding this comment.
imageCache collision risk – multiple images may share an empty fileInfoId
createImageNodeSpec sets a default fileInfoId of ''.
Whenever an inserter forgets to populate this field, every such image will map to the same cache entry, causing alt/size updates to leak between images.
Consider enforcing a unique identifier:
- fileInfoId: { default: '' },
+ fileInfoId: {
+ default: () => crypto.randomUUID(), // always unique
+ },…and/or assert non-emptiness inside getOrCreateImageElement to fail fast during development.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents (early access)
In src/components/text-editor/prosemirror-adapter/plugins/image/node.ts around lines 154 to 168, the imageCache uses fileInfoId as the key, but fileInfoId can be an empty string by default, causing collisions and shared cache entries for multiple images. To fix this, enforce that fileInfoId is non-empty by adding an assertion or check inside getOrCreateImageElement to throw an error if fileInfoId is empty, and consider generating or requiring a unique identifier for each image node to prevent cache collisions.
| toDOM: (node): DOMOutputSpec => { | ||
| if (node.attrs.state === ImageState.FAILED) { | ||
| return createStatusSpan('failed', node, language); | ||
| } else if (node.attrs.state === ImageState.LOADING) { | ||
| return createStatusSpan('loading', node, language); | ||
| if (!isEditorImageState(node.attrs.state)) { | ||
| return; | ||
| } | ||
|
|
||
| let img = imageCache.get(node.attrs.fileInfoId); | ||
| if (img) { | ||
| updateImageElement(img, node); | ||
| } else { | ||
| img = createImageElement(node); | ||
| imageCache.set(node.attrs.fileInfoId, img); | ||
| if (node.attrs.state === 'success') { | ||
| return getOrCreateImageElement(node.attrs.fileInfoId, node); | ||
| } | ||
|
|
||
| return img; | ||
| return createStatusSpanForState(node.attrs.state, node, language); | ||
| }, | ||
| parseDOM: [ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Redundant isEditorImageState check in toDOM
state is already typed and defaulted to a valid EditorImageState; the early return on line 122 can never trigger. Removing it simplifies the function and its test surface:
- if (!isEditorImageState(node.attrs.state)) {
- return;
- }This also aligns with the stricter typing suggested above.
📝 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.
| toDOM: (node): DOMOutputSpec => { | |
| if (node.attrs.state === ImageState.FAILED) { | |
| return createStatusSpan('failed', node, language); | |
| } else if (node.attrs.state === ImageState.LOADING) { | |
| return createStatusSpan('loading', node, language); | |
| if (!isEditorImageState(node.attrs.state)) { | |
| return; | |
| } | |
| let img = imageCache.get(node.attrs.fileInfoId); | |
| if (img) { | |
| updateImageElement(img, node); | |
| } else { | |
| img = createImageElement(node); | |
| imageCache.set(node.attrs.fileInfoId, img); | |
| if (node.attrs.state === 'success') { | |
| return getOrCreateImageElement(node.attrs.fileInfoId, node); | |
| } | |
| return img; | |
| return createStatusSpanForState(node.attrs.state, node, language); | |
| }, | |
| toDOM: (node): DOMOutputSpec => { | |
| if (node.attrs.state === 'success') { | |
| return getOrCreateImageElement(node.attrs.fileInfoId, node); | |
| } | |
| return createStatusSpanForState(node.attrs.state, node, language); | |
| }, |
🤖 Prompt for AI Agents (early access)
In src/components/text-editor/prosemirror-adapter/plugins/image/node.ts between lines 121 and 131, remove the redundant check for isEditorImageState on node.attrs.state in the toDOM function, as state is already typed and guaranteed to be a valid EditorImageState. This means the early return on line 122 is unnecessary. Simplify the function by deleting this check and the associated return statement to reduce complexity and align with stricter typing.
| src: string, | ||
| alt: string, | ||
| width: string, | ||
| maxWidth: string, | ||
| ): string { | ||
| const style = []; | ||
|
|
||
| if (node.attrs.width) { | ||
| style.push(`width: ${node.attrs.width};`); | ||
| } | ||
| if (width) { | ||
| style.push(`width: ${width};`); | ||
| } | ||
|
|
||
| if (node.attrs.maxWidth) { | ||
| style.push(`max-width: ${node.attrs.maxWidth};`); | ||
| } | ||
| if (maxWidth) { | ||
| style.push(`max-width: ${maxWidth};`); | ||
| } | ||
|
|
||
| if (style.length > 0) { | ||
| imageHTML += ` style="${style.join(' ')}"`; | ||
| } | ||
| const styleAttribute = | ||
| style.length > 0 ? ` style="${style.join(' ')}"` : ''; | ||
|
|
||
| imageHTML += ' />'; | ||
| return `<img src="${src}" alt="${alt}"${styleAttribute} />`; | ||
| } | ||
|
|
There was a problem hiding this comment.
Unescaped alt and src attributes enable HTML injection
getImageHTML directly interpolates src and alt into an HTML string without escaping quotes or special characters. A crafted filename like " onerror="alert(1) will break attribute context and execute arbitrary scripts once rendered.
Use a proper encoder (or DOM construction) instead of string concatenation, e.g.:
-import { escape } from 'lodash'; // or a small local util
+import { escape } from 'lodash';
...
- return `<img src="${src}" alt="${alt}"${styleAttribute} />`;
+ return `<img src="${escape(src)}" alt="${escape(alt)}"${styleAttribute} />`;(This applies to the status span HTML too.)
[security]
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents (early access)
In src/components/text-editor/prosemirror-adapter/plugins/image/node.ts lines 78 to 98, the getImageHTML function directly inserts src and alt attributes into an HTML string without escaping special characters, which can lead to HTML injection vulnerabilities. To fix this, implement proper escaping for src and alt values to encode quotes and special characters before including them in the HTML string, or alternatively construct the image element using DOM methods to safely set attributes. Apply the same escaping approach to any other HTML strings generated, such as the status span HTML.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (18)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
136-143: Event-payload construction can be simplified & made future-proofYou can improve this block by passing the whole
EditorLinkobject instead of separate parameters, which will make the function easier to extend when more link properties are added in the future.Apply this diff:
-const openLinkMenu = (view: EditorView, href: string, text: string) => { - const event = new CustomEvent<EditorLink>('open-editor-link-menu', { - detail: { href: href, text: text }, +const openLinkMenu = (view: EditorView, link: EditorLink) => { + const event = new CustomEvent<EditorLink>('open-editor-link-menu', { + detail: link, bubbles: true, composed: true, }); view.dom.dispatchEvent(event); }Then update the call site at line 169:
-openLinkMenu(view, href, text); +openLinkMenu(view, { href, text });src/components/text-editor/text-editor.types.ts (2)
121-131: 🧹 Nitpick (assertive)Add usage example to EditorLink interface documentation
The interface is well-defined, but would benefit from a usage example in the JSDoc.
/** + * Represents a link within the editor. + * + * @example + * ```ts + * const link: EditorLink = { + * href: 'https://example.com', + * text: 'Example Link' + * }; + * ``` * @alpha */ export interface EditorLink {
177-187: 🧹 Nitpick (assertive)Consider using immutability for metadata collections
Making the arrays readonly would prevent accidental mutations that could bypass internal diffing logic.
export interface EditorMetadata { /** * Collection of image elements found in the document */ - images: EditorImage[]; + readonly images: ReadonlyArray<EditorImage>; /** * Collection of link elements found in the document */ - links: EditorLink[]; + readonly links: ReadonlyArray<EditorLink>; }src/components/text-editor/text-editor.tsx (2)
196-204: 🧹 Nitpick (assertive)Add
readonlyto the new event emitter for consistency.Every other
@Event()in this component is declaredprivate readonly. Addingreadonlyhere makes the property immutable and keeps the declaration style uniform.- private readonly metadataChange: EventEmitter<EditorMetadata>; + private readonly metadataChange: EventEmitter<EditorMetadata>;
346-349: 🧹 Nitpick (assertive)Consider bubbling the original event instead of stopping & re-emitting.
Calling
event.stopPropagation()prevents themetadataChangeevent emitted by the internal<limel-prosemirror-adapter>from reaching parent components. You then re-emit a new event with the same name, which works but adds an extra dispatch and resets the event's composed path.If there is no need to manipulate
event.detail, you could simply let the original event bubble out and drop this listener altogether, reducing work and avoiding double events.(Keep the current approach if you intend to manipulate the payload or keep the adapter truly encapsulated.)
src/components/text-editor/utils/metadata-utils.ts (5)
31-33: 🧹 Nitpick (assertive)Simplify image node check and remove redundant attribute check.
The
!!node.attrscheck is redundant becauseNode.attrsis always defined in ProseMirror. You can safely simplify this function:function isImageNode(node: Node): boolean { - return node.type.name === 'image' && !!node.attrs; + return node.type.name === 'image'; }
35-41: 🛠️ Refactor suggestionAdd defensive defaults when extracting image metadata.
To prevent
undefinedvalues from propagating into your metadata, add fallbacks for node attributes:function extractImageMetadata(node: Node): EditorImage { return { - src: node.attrs.src, - state: node.attrs.state, - fileInfoId: node.attrs.fileInfoId, + src: node.attrs.src ?? '', + state: node.attrs.state ?? 'loading', + fileInfoId: node.attrs.fileInfoId ?? '', }; }
47-54: 🛠️ Refactor suggestionAdd nullish coalescing for link metadata extraction.
To prevent
undefinedvalues in your metadata and frequency-map keys, include safe fallbacks:function extractLinkMetadata(node: Node): EditorLink[] { return node.marks .filter((mark) => mark.type.name === 'link' && mark.attrs) .map((mark) => ({ - href: mark.attrs.href, - text: node.text, + href: mark.attrs.href ?? '', + text: node.text ?? '', })); }
111-115: 🛠️ Refactor suggestionProtect against undefined values in frequency map keys.
Template-string concatenation would inject the literal 'undefined' for missing values, causing incorrect equality comparisons. Add nullish coalescing:
images.forEach((image) => { - const key = `${image.fileInfoId}|${image.state}|${image.src}`; + const key = `${image.fileInfoId ?? ''}|${image.state ?? ''}|${image.src ?? ''}`; countMap.set(key, (countMap.get(key) || 0) + 1); });
126-130: 🛠️ Refactor suggestionSame protection needed for link frequency map keys.
Apply the same nullish coalescing pattern here:
links.forEach((link) => { - const key = `${link.href}|${link.text}`; + const key = `${link.href ?? ''}|${link.text ?? ''}`; countMap.set(key, (countMap.get(key) || 0) + 1); });src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (2)
145-148: Implementation needed for actual image removal.Currently,
removeImageonly logs the removal. In a real implementation, this method would need to make API calls to remove the image from external storage:-private removeImage(image: EditorImage) { +private async removeImage(image: EditorImage): Promise<void> { // Remove image from external file storage if desired. console.log(`Image removed: ${image.fileInfoId}`); + // Example implementation: + // try { + // await this.fileStorageApi.deleteFile(image.fileInfoId); + // } catch (error) { + // console.error(`Failed to delete image ${image.fileInfoId}:`, error); + // } }
115-130: 🛠️ Refactor suggestionMake the metadata change handler async-safe.
The
removeImagemethod is likely to perform I/O operations in a real implementation, but the current code doesn't handle promises properly. Make the flow async-safe:-private handleMetadataChange = ( - event: LimelTextEditorCustomEvent<EditorMetadata>, -) => { +private handleMetadataChange = async ( + event: LimelTextEditorCustomEvent<EditorMetadata>, +) => { const removedImages = this.getRemovedImages( this.metadata, event.detail, ); - removedImages.forEach((image) => { - if (image.state === 'success') { - this.removeImage(image); - } - }); + await Promise.all( + removedImages + .filter((image) => image.state === 'success') + .map((image) => this.removeImage(image)) + ); this.metadata = event.detail; };And update the
removeImagemethod to return a Promise:-private removeImage(image: EditorImage) { +private async removeImage(image: EditorImage): Promise<void> { // Remove image from external file storage if desired. console.log(`Image removed: ${image.fileInfoId}`); }src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (4)
78-98:⚠️ Potential issueEscape
src&altto prevent XSS (still unresolved)
getImageHTMLdirectly interpolatessrcandaltinto the markup, so a malicious filename like
" onerror="alert(1)breaks out of the attribute context and executes arbitrary code.This was pointed out in an earlier review, but the vulnerable concatenation remains unchanged.
Please HTML-escape both attributes (or build the element with DOM APIs).
122-128: 🧹 Nitpick (assertive)Redundant state-validation in
toDOM
stateis already typed asEditorImageStateand defaults to'success'; the earlyisEditorImageStatecheck therefore never fails.
Removing it will simplify the code and avoid theundefinedreturn that violatesDOMOutputSpec.
150-152: 🧹 Nitpick (assertive)
isEditorImageStateis too broad for most callersBecause the guard returns
truefor'success', helpers that merely want to detect placeholder states ('loading' | 'failed') have to add an extra check, which is easy to forget (cf. the serializer bug fixed earlier).
Consider narrowing the guard or renaming it to reflect the wider semantics.
154-168:⚠️ Potential issue
imageCachekey-collision risk whenfileInfoIdis emptyIf an inserter forgets to populate
fileInfoId, multiple images share the same cache entry ('').
At minimum, assert non-emptiness before caching; ideally generate a UUID by default to guarantee uniqueness.src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (2)
159-160: 🧹 Nitpick (assertive)Extract reusable constant for empty metadata
private metadata: EditorMetadata = { images: [], links: [] };recreates the same object for every instance and obscures intent.
A named constant (e.g.EMPTY_METADATA) would improve readability and avoid accidental mutation.
518-520: 🧹 Nitpick (assertive)Suppress deprecation warning when emitting
imageRemoved
imageRemovedis kept for backwards-compatibility but is deprecated.
Add the standard ESLint suppression comment (// eslint-disable-next-line sonarjs/deprecation) immediately above theemitto silence CI noise.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (10)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx(3 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts(5 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/node.ts(4 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/view.ts(2 hunks)src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts(2 hunks)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx(8 hunks)src/components/text-editor/text-editor.tsx(4 hunks)src/components/text-editor/text-editor.types.ts(3 hunks)src/components/text-editor/utils/metadata-utils.spec.ts(1 hunks)src/components/text-editor/utils/metadata-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`**/*.{ts,tsx}`: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
**/*.{ts,tsx}: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
src/components/text-editor/text-editor.types.tssrc/components/text-editor/utils/metadata-utils.tssrc/components/text-editor/prosemirror-adapter/plugins/link-plugin.tssrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsxsrc/components/text-editor/prosemirror-adapter/plugins/image/node.tssrc/components/text-editor/utils/metadata-utils.spec.tssrc/components/text-editor/text-editor.tsxsrc/components/text-editor/prosemirror-adapter/plugins/image/inserter.tssrc/components/text-editor/prosemirror-adapter/plugins/image/view.ts
`**/*.tsx`: Our `.tsx` files are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from the `render` method, they will sometime...
**/*.tsx: Our.tsxfiles are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from therendermethod, they will sometimes wrap them in an array literal. In these cases, rather than recommending they addkeyproperties to the elements, recommend removing the hardcoded array literal. Recommend wrapping the elements in StencilJS's special<Host>element.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsxsrc/components/text-editor/text-editor.tsx
`**/*.{tsx,scss}`: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
**/*.{tsx,scss}: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsxsrc/components/text-editor/text-editor.tsx
`src/components/**/examples/**/*.{ts,tsx}`: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is p...
src/components/**/examples/**/*.{ts,tsx}: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is publically exported by lime-elements, the import should be made from@limetech/lime-elements. If they import something from another file inside theexamplefolder, the import should use a relative path.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
🧠 Learnings (2)
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
Learnt from: FredrikWallstrom
PR: Lundalogik/lime-elements#3464
File: src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts:58-67
Timestamp: 2025-03-04T14:44:40.706Z
Learning: In the image inserter plugin for the text editor's ProseMirror adapter, only standard image nodes (where node.type.name === 'image') are intentionally handled, as custom image-like nodes are not within the current scope.
src/components/text-editor/prosemirror-adapter/plugins/image/view.ts (1)
Learnt from: FredrikWallstrom
PR: Lundalogik/lime-elements#3464
File: src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts:58-67
Timestamp: 2025-03-04T14:44:40.706Z
Learning: In the image inserter plugin for the text editor's ProseMirror adapter, only standard image nodes (where node.type.name === 'image') are intentionally handled, as custom image-like nodes are not within the current scope.
🧬 Code Graph Analysis (9)
src/components/text-editor/utils/metadata-utils.ts (1)
src/components/text-editor/text-editor.types.ts (3)
EditorMetadata(177-187)EditorImage(102-116)EditorLink(121-131)
src/components/text-editor/prosemirror-adapter/plugins/link-plugin.ts (1)
src/components/text-editor/text-editor.types.ts (1)
EditorLink(121-131)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (1)
src/components/text-editor/text-editor.types.ts (2)
EditorMetadata(177-187)EditorImage(102-116)
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (4)
src/components/text-editor/text-editor.types.ts (3)
EditorMetadata(177-187)EditorImage(102-116)EditorLink(121-131)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
createImageInserterPlugin(13-29)src/components/text-editor/utils/metadata-utils.ts (2)
getMetadataFromDoc(13-29)hasMetadataChanged(64-73)src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (1)
imageCache(7-7)
src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (1)
src/components/text-editor/text-editor.types.ts (1)
EditorImageState(97-97)
src/components/text-editor/utils/metadata-utils.spec.ts (2)
src/components/text-editor/utils/metadata-utils.ts (2)
getMetadataFromDoc(13-29)hasMetadataChanged(64-73)src/components/text-editor/text-editor.types.ts (1)
EditorMetadata(177-187)
src/components/text-editor/text-editor.tsx (2)
src/interface.ts (1)
EventEmitter(16-16)src/components/text-editor/text-editor.types.ts (2)
EditorImage(102-116)EditorMetadata(177-187)
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (2)
src/components/text-editor/text-editor.types.ts (1)
EditorImageState(97-97)src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (1)
ImageNodeAttrs(100-107)
src/components/text-editor/prosemirror-adapter/plugins/image/view.ts (1)
src/components/text-editor/text-editor.types.ts (1)
EditorImageState(97-97)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Build
🔇 Additional comments (22)
src/components/text-editor/prosemirror-adapter/plugins/image/view.ts (2)
177-185: Great refactoring of state handling logic!Using a map lookup pattern rather than a chain of if-else statements is more maintainable and efficient. This approach scales better if new states are added and makes the code easier to read.
189-191: Consistent string literal usage for state comparisonThe update to use string literals rather than enum values maintains consistency with the new typing system throughout the codebase.
src/components/text-editor/text-editor.types.ts (2)
97-97: String union type provides good type safetyUsing a string union type for
EditorImageStateworks well with TypeScript's type system and improves readability compared to using numeric enums.
102-116: Consistent naming with "Editor" prefixRenaming the interface to follow a consistent naming convention with "Editor" prefix helps distinguish editor-specific types from general-purpose ones.
src/components/text-editor/utils/metadata-utils.spec.ts (3)
37-74: Well-structured test for image extractionThe test thoroughly verifies that images are correctly extracted with all relevant properties.
76-122: Comprehensive test for link extractionGood test coverage for link extraction, verifying that both href and text are correctly captured.
169-295: Excellent test coverage for metadata comparisonsThe tests cover all important scenarios for metadata comparison: count differences, content differences, duplicates, ordering, and identical content. This will ensure robust change detection.
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (6)
1-8: Appropriate imports for the refactored metadata handling.The imports have been correctly updated to reflect the new architecture, with
EditorImageStatereplacing the previous enum type and the appropriate import forImageNodeAttrs.
13-15: Function signature simplified by removing the image removal callback.The plugin's signature is properly simplified by removing the
imageRemovedCallbackparameter, aligning with the shift to metadata-based change detection rather than direct event callbacks.
49-54: Good refactoring with extracted image node attribute creation.The image node attribute creation is now delegated to a helper function, which improves code maintainability and consistency.
69-74: Consistent use of the helper function across all image creation paths.The same helper function is consistently used here for the success state, which ensures uniform attribute handling.
93-98: Consistent use of the helper function for error state.The same helper function is properly used for the failed state, completing the consistent pattern across all image states.
110-121: 🧹 Nitpick (assertive)Well-structured helper function for creating image node attributes.
The new
createImageNodeAttrsfunction centralizes attribute creation logic, but consider adding nullish coalescing operators for defensive coding:function createImageNodeAttrs( src: string, fileInfo: FileInfo, state: EditorImageState, ): ImageNodeAttrs { return { - src: src, - alt: fileInfo.filename, - fileInfoId: fileInfo.id, + src: src ?? '', + alt: fileInfo.filename ?? '', + fileInfoId: fileInfo.id ?? '', state: state, }; }Likely an incorrect or invalid review comment.
src/components/text-editor/text-editor.tsx (4)
10-12: Appropriate imports for the new metadata types.The component correctly imports the required types for the new metadata-based approach.
190-195: Well-documented deprecation for the image removal event.The deprecation notice is clear and provides appropriate guidance to use the new
metadataChangeevent instead.
283-283: Correctly wired up the new metadata change event.The component now properly listens to metadata changes from the adapter.
351-355: Correct handling of the deprecated event with ESLint suppression.The handler properly includes an ESLint disable comment to suppress deprecation warnings while maintaining backward compatibility.
src/components/text-editor/utils/metadata-utils.ts (1)
1-29: Well-structured metadata extraction function with clear documentation.The
getMetadataFromDocfunction provides a clean, modular approach to extracting metadata from ProseMirror documents. The implementation is clear and the documentation is thorough.src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (4)
5-9: Correct imports for the new metadata types.The example component correctly imports the required types for the new metadata-based approach.
45-45: Properly initialized metadata state.The component correctly initializes an empty metadata object to track the current state.
54-54: Event listener updated to use the new metadata change event.The component now listens to metadata changes instead of direct image removal events, aligning with the new architecture.
132-143: Efficient implementation for finding removed images.The use of a
Setfor tracking image IDs provides efficient lookups when determining which images have been removed.
| function createTestSchema() { | ||
| return new Schema({ | ||
| nodes: { | ||
| doc: { content: 'block+' }, | ||
| paragraph: { group: 'block', content: 'inline*' }, | ||
| text: { group: 'inline' }, | ||
| image: { | ||
| group: 'inline', | ||
| inline: true, | ||
| attrs: { | ||
| src: { default: '' }, | ||
| alt: { default: '' }, | ||
| fileInfoId: { default: '' }, | ||
| state: { default: '' }, | ||
| }, | ||
| }, | ||
| }, | ||
| marks: { | ||
| link: { | ||
| attrs: { | ||
| href: { default: '' }, | ||
| title: { default: '' }, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding tests for edge cases
The test suite is comprehensive, but would benefit from tests covering edge cases like empty documents or documents with no images/links.
You could add a test like:
test('getMetadataFromDoc should handle empty documents', () => {
const schema = createTestSchema();
const doc = createTestDoc(schema, {
type: 'doc',
content: [
{
type: 'paragraph',
content: [],
},
],
});
const metadata = getMetadataFromDoc(doc);
expect(metadata.images.length).toBe(0);
expect(metadata.links.length).toBe(0);
});🤖 Prompt for AI Agents (early access)
In src/components/text-editor/utils/metadata-utils.spec.ts around lines 5 to 31, the test suite lacks coverage for edge cases such as empty documents or documents without images or links. Add new test cases that create empty documents or documents with no images/links using the existing createTestSchema and createTestDoc helpers. Then, verify that getMetadataFromDoc returns empty arrays for images and links in these scenarios to ensure robust handling of edge cases.
| function createTestDoc(schema: Schema, content: any): Node { | ||
| return schema.nodeFromJSON(content); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding type annotations for improved clarity
Adding explicit type annotations to the content parameter would improve code readability and type safety.
-function createTestDoc(schema: Schema, content: any): Node {
+function createTestDoc(schema: Schema, content: Record<string, any>): Node {
return schema.nodeFromJSON(content);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function createTestDoc(schema: Schema, content: any): Node { | |
| return schema.nodeFromJSON(content); | |
| } | |
| function createTestDoc(schema: Schema, content: Record<string, any>): Node { | |
| return schema.nodeFromJSON(content); | |
| } |
🤖 Prompt for AI Agents (early access)
In src/components/text-editor/utils/metadata-utils.spec.ts around lines 33 to 35, the function createTestDoc has a parameter content typed as any, which reduces type safety and clarity. Update the content parameter with a more specific type annotation that accurately reflects the expected structure of the JSON content used to create a Node, such as a defined interface or a more precise type instead of any, to improve readability and maintainability.
…anges (images and links) Deprecated the `imageRemoved`. Use the `metadataChange` event instead to track image removals.
|
🎉 This PR is included in version 38.12.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
6 participants
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Deprecations
Bug Fixes
Tests
Connected: https://github.com/Lundalogik/crm-client/issues/38
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: