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 6 commits into
Conversation
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughA shared ProseMirror schema and plugin builder were extracted into ChangesProseMirror editor configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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/ |
john-traas
force-pushed
the
text-editor-real-stack-tests
branch
from
b817bc0 to
74f39e1
Compare
john-traas
force-pushed
the
text-editor-real-stack-tests
branch
from
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.
john-traas
force-pushed
the
text-editor-real-stack-tests
branch
2 times, most recently
from
3e31c98 to
eedd1f9
Compare
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>
john-traas
force-pushed
the
text-editor-real-stack-tests
branch
from
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.
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.