refactor(text-editor): extract editor-config to enable real-stack testing - #4139
refactor(text-editor): extract editor-config to enable real-stack testing#4139john-traas wants to merge 9 commits into
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 (2)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughA shared ProseMirror schema and plugin builder were extracted into ChangesProseMirror editor configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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-4139/ |
b817bc0 to
74f39e1
Compare
719d673 to
bc1e2e9
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: 1
🤖 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/prosemirror-adapter.tsx`:
- Around line 223-245: The clear() method in ProseMirrorAdapter is canceling a
pending debounced change too early, which can drop a legitimate change('')
emission when the editor is already empty. Update clear() so it first checks the
serialized content state, and only cancels changeEmitter / resets changeWaiting
if it will actually call updateView(''); use the clear(), changeEmitter,
changeWaiting, and updateView symbols to keep the behavior aligned with
handleTransaction and lastEmittedValue.
🪄 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: d8535a0c-e026-4419-802b-d81fdab531a0
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsrc/components/text-editor/prosemirror-adapter/editor-config.spec.tssrc/components/text-editor/prosemirror-adapter/editor-config.tssrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or 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: 1
🤖 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/prosemirror-adapter.tsx`:
- Around line 223-245: The clear() method in ProseMirrorAdapter is canceling a
pending debounced change too early, which can drop a legitimate change('')
emission when the editor is already empty. Update clear() so it first checks the
serialized content state, and only cancels changeEmitter / resets changeWaiting
if it will actually call updateView(''); use the clear(), changeEmitter,
changeWaiting, and updateView symbols to keep the behavior aligned with
handleTransaction and lastEmittedValue.
🪄 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: d8535a0c-e026-4419-802b-d81fdab531a0
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsrc/components/text-editor/prosemirror-adapter/editor-config.spec.tssrc/components/text-editor/prosemirror-adapter/editor-config.tssrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
🛑 Comments failed to post (1)
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (1)
223-245: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
clear()cancels a pending real change before checking if it's a no-op.The pending debounced
changeis cancelled unconditionally wheneverchangeWaitingis true, before checking whether the current content is already empty. If a user just deleted all content (socurrentContent === ''and a debouncedchange('')is pending) and a consumer callsclear()right at that moment, this method cancels that legitimate pending emission and then returns early (since content is already empty) — the consumer never receives thechange('')event, even thoughlastEmittedValuewas already updated internally inhandleTransaction. This can leave a consumer's mirrored value permanently stale.Reorder so the cancellation only happens once we know
clear()will actually do work:🐛 Proposed fix
`@Method`() public async clear(): Promise<void> { if (!this.view) { return; } - // Drop any pending debounced change so it cannot fire after this - // clear and resurrect the old content. - if (this.changeWaiting) { - this.changeEmitter.cancel(); - this.changeWaiting = false; - } - const currentContent = this.contentConverter.serialize( this.view, this.schema ); if (currentContent === '') { return; } + // Drop any pending debounced change so it cannot fire after this + // clear and resurrect the old content. + if (this.changeWaiting) { + this.changeEmitter.cancel(); + this.changeWaiting = false; + } + await this.updateView(''); }📝 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.`@Method`() public async clear(): Promise<void> { if (!this.view) { return; } const currentContent = this.contentConverter.serialize( this.view, this.schema ); if (currentContent === '') { return; } // Drop any pending debounced change so it cannot fire after this // clear and resurrect the old content. if (this.changeWaiting) { this.changeEmitter.cancel(); this.changeWaiting = false; } await this.updateView(''); }🤖 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/prosemirror-adapter.tsx` around lines 223 - 245, The clear() method in ProseMirrorAdapter is canceling a pending debounced change too early, which can drop a legitimate change('') emission when the editor is already empty. Update clear() so it first checks the serialized content state, and only cancels changeEmitter / resets changeWaiting if it will actually call updateView(''); use the clear(), changeEmitter, changeWaiting, and updateView symbols to keep the behavior aligned with handleTransaction and lastEmittedValue.
3e31c98 to
eedd1f9
Compare
6af6cc6 to
c499fb6
Compare
rasmusflomen
left a comment
rasmusflomen
left a comment
There was a problem hiding this comment.
Just one small question, nothing blocking. Other than that it's a clean, behaviour-preserving extraction — I diffed the moved code against the old method bodies and node/mark assembly plus plugin order match exactly. Nice to finally have the link-before-image paste ordering locked down by a real-stack test. Great work, John!
| import { Languages } from '../../date-picker/date.types'; | ||
| import { TriggerCharacter, InlineImages } from '../text-editor.types'; | ||
|
|
||
| type ContentType = 'markdown' | 'html'; |
There was a problem hiding this comment.
Not blocking, just a thought: now that this file names the type, could the adapter import ContentType from here instead of repeating the 'markdown' | 'html' literal at prosemirror-adapter.tsx:70? That way there's one definition and the two can't drift apart. Fine to leave since the literal predates this PR.
There was a problem hiding this comment.
Thx for the review and good suggestion. ⚡ 034ce61
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/editor-config.spec.ts`:
- Around line 109-148: Update the linkPlugin and imagePlugin assertions in the
handlePaste tests to use optional chaining when accessing props and invoking
handlePaste. Preserve the existing toBeDefined and falsy runtime assertions
while avoiding unsafe access to the possibly undefined plugins and optional
handlePaste handlers.
🪄 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: 51e00026-b744-4de7-bbd0-14291ba82aff
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsrc/components/text-editor/prosemirror-adapter/editor-config.spec.tssrc/components/text-editor/prosemirror-adapter/editor-config.tssrc/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
rasmusflomen
left a comment
rasmusflomen
left a comment
There was a problem hiding this comment.
Nice follow-up on the ContentType extraction — pulling it into a named type means the two sites can't drift apart. Still a clean, behaviour-preserving refactor and the real-stack test is a good addition. Great work, John!
| // @beta | ||
| export interface LimelProsemirrorAdapter { | ||
| "clear": () => Promise<void>; | ||
| "contentType": 'markdown' | 'html'; |
There was a problem hiding this comment.
this won't be a breaking change, right?
There was a problem hiding this comment.
No, this should not be a breaking change. The accepted values and runtime behavior remain exactly the same: 'markdown' | 'html'. The only difference is that the inline union has been extracted into the shared ContentType type, giving us a single source of truth for the type definition.
rasmusflomen
left a comment
rasmusflomen
left a comment
There was a problem hiding this comment.
Just confirming the ContentType question — extracting the inline union into a named type is structurally identical, so no breaking change there. Clean, behaviour-preserving extraction and the real-stack test locking down link-before-image paste order is a nice addition. Great work, John!
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
a91a012 to
ab4ae79
Compare
What
Extracts the text editor's schema and plugin assembly out of the
limel-prosemirror-adaptercomponent into a pure, importable module, and adds an integration test that exercises the real editor stack.initializeSchema()/createEditorState()(private methods on the adapter) → purebuildEditorSchema()/buildEditorPlugins()ineditor-config.ts. The component delegates to them; plugin order and behaviour are preserved exactly.editor-config.spec.tsbuilds the production schema + the real ordered plugin list and asserts, using the officialprosemirror-test-builder:handlePasteordering contract (ProseMirror is first-truthy-wins) — guarding against a new/changed plugin silently shadowing another on a shared event;cleanscript (shx rm -rf .stencil dist www) and theprosemirror-test-builderdev dependency.Why
The editor is now used across the CRM, so we need regression coverage for cross-plugin interactions (paste / keymap / transaction chains). Those only surface when the real, full plugin set runs against the real schema — previously impossible because the schema and plugin list were private to the component. This unblocks that, tests against source (immune to stale-
distissues), and needs no bespoke test-harness suite.Verification
npm run test:spec: 884/884 pass (incl. the new spec)🤖 Generated with Claude Code
Summary by CodeRabbit
cleanscript to remove generated build/output directories.