feat(text-editor): add inlineImages config for inline image handling#4150
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR adds ChangesInline images feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4150/ |
FredrikWallstrom
force-pushed
the
poc/inline-images-component-markup
branch
from
496df37 to
6c2d190
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts`:
- Around line 328-340: The paste handling in the image inserter still falls back
to the legacy imagePasted event when inlineImages is configured but has no
upload handler. Update the logic around runInlineImageUpload in the inserter
flow so that the presence of an InlineImageTag config itself suppresses the
imagePasted dispatch, and only allow the legacy event when inlineImages is not
configured at all. Use the inlineImages, inlineImages?.upload, and imagePasted
branches to locate and adjust the decision point.
In `@src/components/text-editor/prosemirror-adapter/plugins/image/node.ts`:
- Around line 263-277: The inline image parser in the image node plugin
currently accepts the configured tag even when `image-id` is missing, which lets
invalid tags reach `tag.getUrl('')` inside the `rules.push`/`getAttrs` path.
Update the `getAttrs` logic in
`src/components/text-editor/prosemirror-adapter/plugins/image/node.ts` to reject
tags without a valid `IMAGE_ID_ATTRIBUTE` value before building
`ImageNodeAttrs`, so the parser does not create a success image or throw during
editor parsing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 01966e36-bcef-4989-a3d9-ea4e30f23baa
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (11)
src/components/text-editor/examples/text-editor-with-inline-images-base64.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-custom-tag.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/plugins/image/inserter.tssrc/components/text-editor/prosemirror-adapter/plugins/image/node.tssrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsxsrc/components/text-editor/text-editor.tsxsrc/components/text-editor/text-editor.types.tssrc/components/text-editor/utils/content-type-converter.tssrc/components/text-editor/utils/html-converter.tssrc/components/text-editor/utils/markdown-converter.ts
💤 Files with no reviewable changes (1)
- src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
FredrikWallstrom
force-pushed
the
poc/inline-images-component-markup
branch
from
6c2d190 to
89daecb
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
18-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew private helpers break up the two exported functions.
runInlineImageUploadandreplaceThumbnailWithInlineImage(private helpers, Lines 37-106) are now sandwiched between the exportedcreateImageInserterPlugin(Lines 18-35) and the exportedimageInserterFactory(Lines 108-119). Moving the helpers below both exported functions would keep the public API surfaced first.Based on learnings, "apply the 'news article principle': put the most important public-facing functions at the top of each file... ensure the exported/public functions come before internal helpers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts` around lines 18 - 119, The internal helpers are interrupting the public API order in this module. Move `runInlineImageUpload` and `replaceThumbnailWithInlineImage` below both exported symbols, `createImageInserterPlugin` and `imageInserterFactory`, so the file presents the public functions first and keeps private implementation details after them.
♻️ Duplicate comments (1)
src/components/text-editor/utils/content-type-converter.ts (1)
3-4: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCross-file attribute-name duplication with
node.ts.
buildContentWhitelistre-declares the inline-image attribute names ('image-id','width','height','alt') independently ofIMAGE_ID_ATTRIBUTEand the attribute set built innode.ts'sgetInlineImageHTML/buildInlineImageDOMAttrs. See the corresponding comment onsrc/components/text-editor/prosemirror-adapter/plugins/image/node.ts(lines 131-166) for the consolidated fix suggestion — exporting a shared constant fromnode.tswould let this whitelist stay in sync automatically.Also applies to: 18-43
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/text-editor/utils/content-type-converter.ts` around lines 3 - 4, buildContentWhitelist is duplicating the inline-image attribute names instead of reusing the shared source from node.ts, so keep the whitelist in sync by exporting and consuming the same inline-image attribute constant/set used by getInlineImageHTML and buildInlineImageDOMAttrs. Update buildContentWhitelist to reference the shared definition (including IMAGE_ID_ATTRIBUTE and the other inline image attrs) rather than hardcoding 'image-id', 'width', 'height', and 'alt'.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/components/text-editor/examples/text-editor-with-inline-images-custom-tag.tsx`:
- Around line 73-88: The uploadImage helper in
text-editor-with-inline-images-custom-tag.tsx creates object URLs with
URL.createObjectURL but never cleans them up. Update the image storage flow
around uploadImage and the store usage so any replaced or removed entry has its
URL revoked with URL.revokeObjectURL, and add a disconnectedCallback on the
component to revoke all remaining stored object URLs when the editor is torn
down.
In `@src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts`:
- Around line 62-72: The upload failure in the image inserter is being swallowed
by the bare catch, so add error handling in inserter.ts around the upload(file)
call inside the thumbnail insertion flow. Update the catch in the inserter logic
to capture the thrown error and log it or emit it through the existing
inserter/event path before calling inserter.insertFailedThumbnail(), using the
surrounding symbols upload, replaceThumbnailWithInlineImage, and
inserter.insertFailedThumbnail to locate the fix.
- Around line 47-73: The inline image upload flow in runInlineImageUpload has no
timeout or cancellation around inlineImages.upload(file), so a hung promise can
leave the thumbnail loading forever. Update runInlineImageUpload to race the
upload against a timeout (or cancellation signal) and treat timeout as a failure
path that falls through to inserter.insertFailedThumbnail(), while keeping the
existing success path through replaceThumbnailWithInlineImage.
In `@src/components/text-editor/prosemirror-adapter/plugins/image/node.ts`:
- Around line 131-148: The inline image HTML path is using a custom escape
helper that static analysis flags as risky; replace the bespoke
escapeAttributeValue logic in node.ts with a vetted escaping utility already
used in the codebase, or a small well-tested encoder such as he.encode. Update
getInlineImageHTML to use that shared helper for IMAGE_ID_ATTRIBUTE, width,
height, and alt so attribute escaping is centralized and future-safe.
- Around line 131-166: The inline image attribute names are duplicated and
`buildInlineImageDOMAttrs` hardcodes `'image-id'` instead of reusing
`IMAGE_ID_ATTRIBUTE`, which can drift from `getInlineImageHTML` and
`createImageParseDOM`. Update `buildInlineImageDOMAttrs` to use the shared
constant, and centralize the inline image attribute list so
`content-type-converter.ts`’s `buildContentWhitelist` can reuse the same source
of truth for `'image-id'`, `'width'`, `'height'`, and `'alt'`.
In `@src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx`:
- Around line 118-128: The doc comment for the `inlineImages` prop in
`ProsemirrorAdapter` is too narrow because it only describes the tag-based
persistence shape and omits the `InlineImageSrc` variant. Update the JSDoc on
`inlineImages` to match the shape-agnostic wording used in `text-editor.tsx`, so
it clearly covers both tag-based and plain `<img src>` inline image persistence
without implying only file-id tags are supported.
---
Outside diff comments:
In `@src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts`:
- Around line 18-119: The internal helpers are interrupting the public API order
in this module. Move `runInlineImageUpload` and
`replaceThumbnailWithInlineImage` below both exported symbols,
`createImageInserterPlugin` and `imageInserterFactory`, so the file presents the
public functions first and keeps private implementation details after them.
---
Duplicate comments:
In `@src/components/text-editor/utils/content-type-converter.ts`:
- Around line 3-4: buildContentWhitelist is duplicating the inline-image
attribute names instead of reusing the shared source from node.ts, so keep the
whitelist in sync by exporting and consuming the same inline-image attribute
constant/set used by getInlineImageHTML and buildInlineImageDOMAttrs. Update
buildContentWhitelist to reference the shared definition (including
IMAGE_ID_ATTRIBUTE and the other inline image attrs) rather than hardcoding
'image-id', 'width', 'height', and 'alt'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 39d53d14-48d2-4df1-8023-37e153250e44
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (11)
src/components/text-editor/examples/text-editor-with-inline-images-base64.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-custom-tag.tsxsrc/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsxsrc/components/text-editor/prosemirror-adapter/plugins/image/inserter.tssrc/components/text-editor/prosemirror-adapter/plugins/image/node.tssrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsxsrc/components/text-editor/text-editor.tsxsrc/components/text-editor/text-editor.types.tssrc/components/text-editor/utils/content-type-converter.tssrc/components/text-editor/utils/html-converter.tssrc/components/text-editor/utils/markdown-converter.ts
💤 Files with no reviewable changes (1)
- src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
| private readonly uploadImage = (file: File): Promise<string> => { | ||
| // Simulate uploading to external storage and returning the stored id. | ||
| return new Promise((resolve, reject) => { | ||
| setTimeout(() => { | ||
| if (this.uploadImageFails) { | ||
| reject(new Error('Server error')); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| const id = crypto.randomUUID(); | ||
| this.store.set(id, URL.createObjectURL(file)); | ||
| resolve(id); | ||
| }, 2000); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Object URLs created via URL.createObjectURL are never revoked.
Each simulated upload calls URL.createObjectURL(file) and stores the result, but nothing ever calls URL.revokeObjectURL for replaced/removed entries. In this demo it's a minor leak, but since this is example code other consumers may copy, it's worth showing the cleanup pattern (e.g., revoke previous URL for a given id, or revoke on component disconnect).
♻️ Proposed fix
const id = crypto.randomUUID();
- this.store.set(id, URL.createObjectURL(file));
+ const url = URL.createObjectURL(file);
+ this.store.set(id, url);
resolve(id);And revoke stored URLs in a disconnectedCallback if the component is expected to be destroyed/recreated repeatedly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/components/text-editor/examples/text-editor-with-inline-images-custom-tag.tsx`
around lines 73 - 88, The uploadImage helper in
text-editor-with-inline-images-custom-tag.tsx creates object URLs with
URL.createObjectURL but never cleans them up. Update the image storage flow
around uploadImage and the store usage so any replaced or removed entry has its
URL revoked with URL.revokeObjectURL, and add a disconnectedCallback on the
component to revoke all remaining stored object URLs when the editor is torn
down.
There was a problem hiding this comment.
Leaving as-is. In this example the stored object URL is exactly what getUrl returns to render the image, so revoking it would break the demo; only disconnect-time cleanup would be valid, which adds noise to a docs example for little gain.
| const runInlineImageUpload = async ( | ||
| view: EditorView, | ||
| file: File, | ||
| base64Data: string, | ||
| fileInfo: FileInfo, | ||
| inlineImages: InlineImages | ||
| ): Promise<void> => { | ||
| const upload = inlineImages.upload; | ||
| if (!upload) { | ||
| return; | ||
| } | ||
|
|
||
| const inserter = imageInserterFactory(view, base64Data, fileInfo); | ||
| inserter.insertThumbnail(); | ||
|
|
||
| try { | ||
| const uploadResult = await upload(file); | ||
| replaceThumbnailWithInlineImage( | ||
| view, | ||
| fileInfo, | ||
| uploadResult, | ||
| inlineImages | ||
| ); | ||
| } catch { | ||
| inserter.insertFailedThumbnail(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No timeout/cancellation around inlineImages.upload(file).
If the consumer's upload promise never settles (hung network request, dropped connection), the thumbnail stays stuck in the loading state indefinitely with no way to recover — the user has no feedback and the paste effectively becomes unusable. Consider racing the upload against a timeout so the flow can fall through to insertFailedThumbnail() in that case.
🕐 Proposed fix
try {
- const uploadResult = await upload(file);
+ const uploadResult = await withTimeout(upload(file), UPLOAD_TIMEOUT_MS);
replaceThumbnailWithInlineImage(
view,
fileInfo,
uploadResult,
inlineImages
);
} catch {
inserter.insertFailedThumbnail();
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts`
around lines 47 - 73, The inline image upload flow in runInlineImageUpload has
no timeout or cancellation around inlineImages.upload(file), so a hung promise
can leave the thumbnail loading forever. Update runInlineImageUpload to race the
upload against a timeout (or cancellation signal) and treat timeout as a failure
path that falls through to inserter.insertFailedThumbnail(), while keeping the
existing success path through replaceThumbnailWithInlineImage.
There was a problem hiding this comment.
Leaving this out. Timeout/cancellation is really the consumer's upload contract — an editor-side timeout would risk failing legitimately slow or large uploads. A never-settling upload is an edge case we'd rather not mask with a hardcoded limit here.
| function escapeAttributeValue(value: string): string { | ||
| return String(value) | ||
| .replaceAll('&', '&') | ||
| .replaceAll('"', '"') | ||
| .replaceAll('<', '<') | ||
| .replaceAll('>', '>'); | ||
| } | ||
|
|
||
| function getInlineImageHTML(attrs: ImageNodeAttrs, tag: string): string { | ||
| const attributes = [ | ||
| `${IMAGE_ID_ATTRIBUTE}="${escapeAttributeValue(attrs.imageId ?? '')}"`, | ||
| attrs.width ? `width="${escapeAttributeValue(attrs.width)}"` : '', | ||
| attrs.height ? `height="${escapeAttributeValue(attrs.height)}"` : '', | ||
| `alt="${escapeAttributeValue(attrs.alt ?? '')}"`, | ||
| ].filter(Boolean); | ||
|
|
||
| return `<${tag} ${attributes.join(' ')}></${tag}>`; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Consider a vetted escaping utility instead of hand-rolled entity replacement.
escapeAttributeValue manually replaces &, ", <, >. It's used correctly here (values are always placed inside double-quoted attributes, so the missing ' escape isn't currently exploitable), but static analysis flags this pattern as risky (CWE-79) since any future change to single-quoted attributes or additional consumers of this helper could reopen an injection vector.
Consider using a small, well-tested escaping utility (e.g. he.encode, or a shared sanitize-html/DOMPurify-based helper already used elsewhere in the codebase) rather than a bespoke replacement chain.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 131-135: Avoid hand-rolled HTML escaping (replacing characters with HTML entities); use a vetted encoder/sanitizer such as DOMPurify or sanitize-html.
Context: String(value)
.replaceAll('&', '&')
.replaceAll('"', '"')
.replaceAll('<', '<')
.replaceAll('>', '>')
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(manual-sanitization-typescript)
[warning] 131-134: Avoid hand-rolled HTML escaping (replacing characters with HTML entities); use a vetted encoder/sanitizer such as DOMPurify or sanitize-html.
Context: String(value)
.replaceAll('&', '&')
.replaceAll('"', '"')
.replaceAll('<', '<')
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(manual-sanitization-typescript)
[warning] 131-133: Avoid hand-rolled HTML escaping (replacing characters with HTML entities); use a vetted encoder/sanitizer such as DOMPurify or sanitize-html.
Context: String(value)
.replaceAll('&', '&')
.replaceAll('"', '"')
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(manual-sanitization-typescript)
[warning] 131-132: Avoid hand-rolled HTML escaping (replacing characters with HTML entities); use a vetted encoder/sanitizer such as DOMPurify or sanitize-html.
Context: String(value)
.replaceAll('&', '&')
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(manual-sanitization-typescript)
[warning] 131-135: Manual HTML entity encoding detected. Avoid manually sanitizing HTML input as it can be error-prone and lead to XSS vulnerabilities. Use dedicated libraries like sanitize-html or DOMPurify for proper HTML sanitization.
Context: String(value)
.replaceAll('&', '&')
.replaceAll('"', '"')
.replaceAll('<', '<')
.replaceAll('>', '>')
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(manual-html-entity-encoding)
[warning] 131-134: Manual HTML entity encoding detected. Avoid manually sanitizing HTML input as it can be error-prone and lead to XSS vulnerabilities. Use dedicated libraries like sanitize-html or DOMPurify for proper HTML sanitization.
Context: String(value)
.replaceAll('&', '&')
.replaceAll('"', '"')
.replaceAll('<', '<')
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(manual-html-entity-encoding)
[warning] 131-135: Manual HTML sanitization detected using string replacement methods. Manual sanitization is error-prone and can be bypassed. Use dedicated HTML sanitization libraries like 'sanitize-html' or 'DOMPurify' instead.
Context: String(value)
.replaceAll('&', '&')
.replaceAll('"', '"')
.replaceAll('<', '<')
.replaceAll('>', '>')
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(manual-html-sanitization)
[warning] 131-134: Manual HTML sanitization detected using string replacement methods. Manual sanitization is error-prone and can be bypassed. Use dedicated HTML sanitization libraries like 'sanitize-html' or 'DOMPurify' instead.
Context: String(value)
.replaceAll('&', '&')
.replaceAll('"', '"')
.replaceAll('<', '<')
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(manual-html-sanitization)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/text-editor/prosemirror-adapter/plugins/image/node.ts` around
lines 131 - 148, The inline image HTML path is using a custom escape helper that
static analysis flags as risky; replace the bespoke escapeAttributeValue logic
in node.ts with a vetted escaping utility already used in the codebase, or a
small well-tested encoder such as he.encode. Update getInlineImageHTML to use
that shared helper for IMAGE_ID_ATTRIBUTE, width, height, and alt so attribute
escaping is centralized and future-safe.
Source: Linters/SAST tools
There was a problem hiding this comment.
Leaving the small helper. As it notes, the values are always placed inside double-quoted attributes so this isn't exploitable, and pulling in an encoder dependency isn't warranted here.
FredrikWallstrom
force-pushed
the
poc/inline-images-component-markup
branch
2 times, most recently
from
60ed000 to
e3568ea
Compare
| return [ | ||
| ...customNodes, | ||
| { | ||
| tagName: inlineImages.tagName, |
There was a problem hiding this comment.
This could be an over-defensive suggestion but tagName is used as a sanitizer whitelist key without validation. If a consumer passes tagName: 'img', this entry replaces the sanitizer's default attribute rules for img — so src is stripped from every existing <img> in content, and all regular images render blank. No error anywhere; just a silent display bug far from its cause. ('script' would likewise become an allowed tag.)
Plausible mistake too: someone wanting plain <img> persistence writes { tagName: 'img', ... } instead of using the InlineImageSrc shape — type-valid, and isInlineImageTag routes it here.
Since the JSDoc already says "Not img", suggest enforcing it:
const CUSTOM_ELEMENT_NAME = /^[a-z][a-z0-9._-]*-[a-z0-9._-]*$/;
if (!CUSTOM_ELEMENT_NAME.test(inlineImages.tagName)) {
console.error(`inlineImages.tagName "${inlineImages.tagName}" is not a valid custom element name; ignoring inlineImages config`);
return customNodes;
}Custom element names must contain a hyphen, so this can never collide with a built-in tag. Warn-and-ignore so a typo degrades to legacy behavior instead of breaking the editor.
There was a problem hiding this comment.
Good catch, thanks — real footgun. Fixed two ways b19e025:
tagName is now typed ${string}-${string} instead of string, so a literal 'img'/'script' is a compile error. And for dynamically-built names that dodge the type, there's a runtime guard — but I put it upstream in the adapter rather than in buildContentWhitelist, since tagName also feeds the node parse rule and the paste inserter. Validating once when the config is read (invalid → treated as absent + a console.error) degrades to legacy handling everywhere, not just the sanitizer.
Used a hyphen check (/^[a-z][a-z0-9._-]*$/ + includes('-')) instead of your single regex — SonarJS flagged that one for backtracking. Same idea though: valid custom element names have a hyphen, built-ins never do.
john-traas
left a comment
john-traas
left a comment
There was a problem hiding this comment.
Looks great! Excellent work!
FredrikWallstrom
force-pushed
the
poc/inline-images-component-markup
branch
from
b19e025 to
75f5961
Compare
Consolidated PR ReviewAutomated 6-agent-review → fix-loop, iteration 1 · 7 dimensions (Lime CRM project detected) PR SummaryAdds an Merge Readiness -- READY TO MERGE ✅This PR is strictly better than
1. Backward Compatibility -- SAFE ✅Additive public surface; legacy path preserved.
2. Code Quality -- GOOD ✅Sound types and error handling; the notable gap is test coverage.
3. Architecture -- GOOD ✅Cleanly additive; legacy and new lifecycles never interleave.
4. Security -- SAFE ✅Strictly safer than main on the serialization path; no new vulnerability.
5. Observability -- GOOD ✅Actionable config-validation and upload-failure logging; a few silent-drop edges.
6. Performance -- GOOD ✅Constant-factor only; the width-guard is a genuine win.
7. Lime Platform Issues -- SAFE ✅LPID catalogue loaded successfully (via internal-docs MCP); the changed code was evaluated against the Frontend (FE) rule category. No LPID findings. The editor performs no HTTP itself —
Overall Verdicts
Top Recommendations
|
Addressed review feedbackPushed a fixup ( Fixed
Not changed
|
Consolidated PR Review — iteration 2Automated 6-agent-review → fix-loop, iteration 2 · 7 dimensions (Lime CRM project detected). Re-review of the current head after the iteration-1 fixup ( PR SummaryAdds an Merge Readiness -- READY TO MERGE ✅Still a strict improvement over
1. Backward Compatibility -- SAFE ✅Public surface still purely additive; internal exports stay internal.
2. Code Quality -- GOOD ✅The iteration-1
3. Architecture -- GOOD ✅Refactor improves cohesion and fixes latent bugs; two documented-intent
4. Security -- SAFE ✅Strict improvement over
5. Observability -- GOOD ✅The iteration-1 logging fixes are sound; two minor
6. Performance -- GOOD ✅No new concerns; the refactor is perf-neutral-to-better.
7. Lime Platform Issues -- SAFE ✅LPID catalogue loaded (internal-docs MCP); only the Frontend (FE) category is in scope for this client-side Stencil library. No LPID findings. The new escaping/HTML-string builders produce serialized editor output (re-sanitized on parse), not a DOM-parsing sink — not FE-10.
Overall Verdicts
Top RecommendationsAll remaining items are
Informational (no action expected): the legacy- |
|
Addressed the review (all Fixed
Not changed
|
Consolidated PR Review — iteration 3Automated 6-agent-review → fix-loop, iteration 3 · 7 dimensions (Lime CRM project detected). Re-review of the current head after fixup PR SummaryAdds an Merge Readiness -- READY TO MERGE ✅Still a strict improvement over
1. Backward Compatibility -- SAFE ✅No significant issues found.
2. Code Quality -- GOOD ✅The iteration-2 comment
3. Architecture -- GOOD ✅No findings; the dimension is clean.
4. Security -- SAFE ✅No findings; clean.
5. Observability -- GOOD ✅The iteration-2 recommendation was implemented soundly — orphaned successful uploads are now locatable by stored id, on the strictly-correct success-but-node-gone branch, leaking nothing sensitive in the intended tag-shape case.
6. Performance -- GOOD ✅Clean. Both delta edits are performance no-ops.
7. Lime Platform Issues -- SAFE ✅LPID catalogue loaded (internal-docs MCP); only the Frontend (FE) category applies to this client-side Stencil library. No LPID findings. The
Overall Verdicts
Top RecommendationsThe single remaining item is
|
Review round 3 addressedFixed the single The payload key was Pushed as fixup |
Consolidated PR Review — iteration 4 (converged ✅)Automated 6-agent-review → fix-loop, iteration 4 · 7 dimensions (Lime CRM project detected). Re-review of the current head after fixup PR SummaryAdds an Merge Readiness -- READY TO MERGE ✅The PR has converged: every dimension is clean this round and there are no outstanding recommendations. It is a strict improvement over
1. Backward Compatibility -- SAFE ✅No significant issues found. The one-line delta is an internal
2. Code Quality -- GOOD ✅No findings. The iteration-3
3. Architecture -- GOOD ✅No findings. The delta is architecturally inert (a log-payload key). The additive-coexistence design, the shared
4. Security -- SAFE ✅No findings. The rename changes only the log key, not the value or any behavior;
5. Observability -- GOOD ✅No findings. The iteration-3 finding is resolved: the shape-neutral
6. Performance -- GOOD ✅No findings. A property-key rename on the rare orphan branch — no allocation, iteration, or hot-path impact. The pre-existing accepted
7. Lime Platform Issues -- SAFE ✅No LPID findings. LPID catalogue loaded (internal-docs MCP); only the Frontend (FE) category applies. The delta is a browser-console diagnostic key rename — no runtime FE surface. Re-checked FE-1..FE-10 (no fetch/HTTP added;
Overall Verdicts
Top RecommendationsNone — the review is clean and the PR has converged. No |
Add an `inlineImages` prop so the editor can own the paste lifecycle
(thumbnail -> upload -> resizable image or failed state) instead of the
consumer sequencing it via the `imagePasted` event.
Two persistence shapes via a discriminated union:
- InlineImageTag ({ tagName, getUrl, upload }) -> `<tagName image-id>`,
an id-only microformat resolved to a URL at render.
- InlineImageSrc ({ upload }) -> plain `<img src>` (upload returns the src).
`isInlineImageTag` narrows the union. The old `imagePasted`/`metadataChange`
API is untouched and used when `inlineImages` is absent, and existing
`<img src>` content round-trips unchanged. Examples migrated to the new config.
Add spec coverage for the DOM-free inline-image units introduced with the `inlineImages` config: - `isInlineImageTag` union narrowing - `buildContentWhitelist`, including the hyphen-rule rejection of a built-in tag name - the `image` node parse/serialize round-trip: id resolution via `getUrl`, the missing/empty `image-id` reject, id-only round-trip stability, attribute escaping and the plain-`<img>` fallback Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
FredrikWallstrom
force-pushed
the
poc/inline-images-component-markup
branch
from
4c019b3 to
4f8a7cc
Compare
|
🎉 This PR is included in version 39.41.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
4 participants
Part of the Shared Files inline-images work for the feed (Closes Lundalogik/crm-client#1148). This is the lime-elements piece — the generic editor support. The CRM-specific wiring (building-blocks, crm-components, limepkg-email) follows in separate PRs, which is why this is a draft.
What
Adds an
inlineImagesconfig so the editor owns the whole paste lifecycle itself — show a thumbnail, run the consumer'supload, then swap in a resizable image (or a failed state) — instead of the consumer driving that through theimagePastedevent.It supports two persistence shapes, modelled as a discriminated union so invalid combinations are compile errors rather than runtime surprises:
InlineImageTag—{ tagName, getUrl, upload }persists<tagName image-id="…">, an id-only microformat that's resolved to a URL at render. Portable; no URL is baked into the stored content.uploadreturns the stored id;getUrlresolves it for display inside the editor.InlineImageSrc—{ upload }persists a plain<img src="…">.uploadreturns the src directly (a URL or a data URI — base64 or external storage both fit here).isInlineImageTagnarrows the union for consumers.Compatibility
The old
imagePasted/imageRemoved/metadataChangeAPI is untouched and is still used wheneverinlineImagesisn't provided, so existing consumers (e.g. solution-grc) keep working. Existing<img src>content round-trips unchanged — only newly pasted images use the new microformat.The two examples were migrated to the new config and renamed to reflect the two shapes (image-source and custom-tag).
Not in this PR
Reconcile-on-save (attaching the uploaded shared files to a limeobject) lives in the consumers/backend and lands separately.
Summary by CodeRabbit
inlineImagessetting (tag-based or src-based persistence).