Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughUpdated menu toggle logic to detect ancestor wraps when toggling node types; exported a table style helper with a widened DOM setter type; and added a comprehensive ProseMirror test utilities suite (state/view/schema/content/commands/events/mocks) with accompanying tests and documentation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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-4012/ |
john-traas
changed the title
john-traas
force-pushed
the
refine-text-editor-test-suite
branch
from
5c23365 to
fd21905
Compare
john-traas
force-pushed
the
refine-text-editor-test-suite
branch
from
fd21905 to
f7611fa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/text-editor/PROSEMIRROR-REFERENCE.md`:
- Line 301: The sentence describing EditorState uses redundant phrasing "at a
point in time"; update the documentation line that reads "EditorState holds the
complete state of the editor at a point in time:" to a tighter phrasing such as
"EditorState holds the complete state of the editor at a point:" or "EditorState
holds the complete state of the editor at a time:"—pick one and replace the
existing phrase to remove the redundancy.
In `@src/components/text-editor/test-setup/content-generator.spec.ts`:
- Around line 111-121: The assertions are currently guarded by if checks and can
vacuously pass; replace those guards with explicit existence assertions before
dereferencing: assert that firstChild and firstChild.firstChild exist (e.g.,
expect(firstChild).toBeDefined(); expect(firstChild.firstChild).toBeDefined())
before finding linkMark, then assert linkMark exists
(expect(linkMark).toBeDefined()) and only after that check linkMark.attrs.href
and linkMark.attrs.title; apply the same pattern to the other identical block
(the second conditional around the link mark) so no assertions run inside an if
without first asserting presence of the nodes.
In `@src/components/text-editor/test-setup/content-generator.ts`:
- Around line 24-31: The helper createDocWithText currently interpolates text
into HTML ( `<p>${text}</p>` ), which wrongly treats input like
"<strong>x</strong>" as markup; instead build the document from Schema nodes so
text is treated literally: in createDocWithText (using createTestSchema() when
schema is absent) create a paragraph node via
schema.nodes.paragraph.createChecked(schema.text(text || '')) and then create
the top-level doc node (schema.nodes.doc.createChecked(...)) and pass that node
into createEditorState rather than passing HTML; update references to Schema,
createTestSchema, createEditorState and createDocWithText accordingly.
In `@src/components/text-editor/test-setup/event-simulator.ts`:
- Around line 44-49: simulateKeyPress() and simulatePaste() currently return the
raw result of domNode.dispatchEvent(event) which is inverted relative to the
documented contract (dispatchEvent returns false when preventDefault() is
called); update both helpers to return event.defaultPrevented instead of the
dispatchEvent return value so they report true when the editor handled the
event; locate the helpers simulateKeyPress and simulatePaste in
event-simulator.ts, stop returning the dispatchEvent result and return the
KeyboardEvent/ClipboardEvent instance's defaultPrevented property to indicate
handling.
In `@src/components/text-editor/test-setup/schema-builder.ts`:
- Around line 6-13: The underline MarkSpec's parseDOM style matchers use
incorrect syntax ('property:value') so they never match inline styles; update
the parseDOM array in the underline spec to use exact-match syntax
('text-decoration=underline' and 'text-decoration-line=underline') so styled
HTML parsed by createDocWithHTML (and related helpers) correctly preserves
underline marks.
In `@src/components/text-editor/TEXT-EDITOR-CONTEXT.md`:
- Around line 67-75: The unlabeled fenced code block that starts with "Base
schema (prosemirror-schema-basic)" in TEXT-EDITOR-CONTEXT.md (and other
unlabeled fences later in the file) should be tagged with a language label
(e.g., ```text or ```ts) to satisfy markdownlint MD040 and improve rendering;
update each unlabeled fence to include an appropriate language label
consistently (pick text for prose lists or ts for code examples) and re-run the
linter to ensure all occurrences are fixed.
🪄 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: fc570d23-7430-48ef-b133-847c3d5578cf
📒 Files selected for processing (19)
src/components/text-editor/PROSEMIRROR-REFERENCE.mdsrc/components/text-editor/TEXT-EDITOR-CONTEXT.mdsrc/components/text-editor/prosemirror-adapter/menu/menu-commands.tssrc/components/text-editor/prosemirror-adapter/plugins/table-plugin.spec.tssrc/components/text-editor/prosemirror-adapter/plugins/table-plugin.tssrc/components/text-editor/test-setup/command-tester.spec.tssrc/components/text-editor/test-setup/command-tester.tssrc/components/text-editor/test-setup/content-generator.spec.tssrc/components/text-editor/test-setup/content-generator.tssrc/components/text-editor/test-setup/editor-state-builder.spec.tssrc/components/text-editor/test-setup/editor-state-builder.tssrc/components/text-editor/test-setup/editor-test-utils.tssrc/components/text-editor/test-setup/editor-view-builder.spec.tssrc/components/text-editor/test-setup/editor-view-builder.tssrc/components/text-editor/test-setup/event-simulator.spec.tssrc/components/text-editor/test-setup/event-simulator.tssrc/components/text-editor/test-setup/schema-builder.spec.tssrc/components/text-editor/test-setup/schema-builder.tssrc/components/text-editor/test-setup/text-editor-test-suite-guide.md
src/components/text-editor/test-setup/content-generator.spec.ts
Outdated
Show resolved
Hide resolved
john-traas
force-pushed
the
refine-text-editor-test-suite
branch
from
f7611fa to
808b9c4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
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/table-plugin.ts (1)
15-18:⚠️ Potential issue | 🟡 MinorNormalize existing
stylebefore appending new declarations.If
attrs.styleis present but does not end with;, the current concatenation can generate invalid CSS (e.g.color: redbackground-color: blue;).Proposed fix
setDOMAttr: (value: string, attrs: Record<string, any>) => { if (value) { - attrs.style = (attrs.style || '') + `${cssProperty}: ${value};`; + const currentStyle = attrs.style ? String(attrs.style).trim() : ''; + const normalizedStyle = currentStyle && !currentStyle.endsWith(';') + ? `${currentStyle};` + : currentStyle; + attrs.style = `${normalizedStyle}${cssProperty}: ${value};`; } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/prosemirror-adapter/plugins/table-plugin.ts` around lines 15 - 18, The setDOMAttr function appends a new CSS declaration to attrs.style without ensuring the existing style string ends with a semicolon, which can produce malformed CSS; update setDOMAttr (the setDOMAttr: (value: string, attrs: Record<string, any>) => { ... } block) to normalize attrs.style before concatenation by trimming it, adding a trailing semicolon if missing, then append `${cssProperty}: ${value};` only when value is truthy so the final style string is always valid.
♻️ Duplicate comments (3)
src/components/text-editor/test-setup/event-simulator.ts (1)
44-49:⚠️ Potential issue | 🟠 MajorReturn handled-ness from
defaultPrevented, notdispatchEvent().
dispatchEvent()returnsfalsewhen a cancelable event was prevented. Both helpers currently return that raw boolean, so a handled key/paste event is reported asfalseeven though the JSDoc promises the opposite.Suggested fix
const event = new KeyboardEvent('keydown', options); - - const domNode = view.dom; - const eventHandled = domNode.dispatchEvent(event); - - return eventHandled; + view.dom.dispatchEvent(event); + return event.defaultPrevented; } @@ Object.defineProperty(pasteEvent, 'clipboardData', { value: dataTransfer, writable: false, }); - - const domNode = view.dom; - const eventHandled = domNode.dispatchEvent(pasteEvent); - - return eventHandled; + view.dom.dispatchEvent(pasteEvent); + return pasteEvent.defaultPrevented; }Also applies to: 72-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/test-setup/event-simulator.ts` around lines 44 - 49, The helpers currently return the raw result of domNode.dispatchEvent(event) (stored in eventHandled), which is false when preventDefault() was called; instead return the handled-ness based on the event's defaultPrevented flag. Replace usage of eventHandled/dispatchEvent() return with reading event.defaultPrevented (i.e., return event.defaultPrevented) after calling domNode.dispatchEvent(event); apply the same fix to the other helper that dispatches the paste event (the second occurrence around lines 72-75) and keep view.dom and the dispatchEvent call intact.src/components/text-editor/TEXT-EDITOR-CONTEXT.md (1)
14-25:⚠️ Potential issue | 🟡 MinorAdd fence languages and normalize blank lines around fenced blocks.
Lines [14-25], [67-75], and [148-156] use unlabeled fenced blocks; add a language label (likely
text) and keep blank lines around fences to satisfy markdownlint (MD040/MD031) consistently.Example fix pattern
-``` +```text limel-text-editor (text-editor.tsx) ... -``` +```Also applies to: 67-75, 148-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/TEXT-EDITOR-CONTEXT.md` around lines 14 - 25, Add a language label (use "text") to each unlabeled fenced block and ensure there is a blank line before and after each fenced block to satisfy markdownlint rules (MD040/MD031); update the fenced blocks that contain the limel-text-editor tree (including the block showing limel-notched-outline, limel-prosemirror-adapter, `#editor`, .toolbar, limel-action-bar, limel-portal, limel-text-editor-link-menu, limel-markdown, and limel-helper-line) as well as the other similar unlabeled blocks mentioned, by replacing ``` with ```text and making sure an empty line precedes and follows each fenced section.src/components/text-editor/PROSEMIRROR-REFERENCE.md (1)
301-301:⚠️ Potential issue | 🟡 MinorTighten redundant phrasing in the EditorState sentence.
Line [301] still uses “at a point in time”; simplify to “at a point” or “at a time” for cleaner wording.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/PROSEMIRROR-REFERENCE.md` at line 301, The sentence describing EditorState is wordy — change "EditorState holds the complete state of the editor at a point in time" to a tighter phrasing such as "EditorState holds the complete state of the editor at a point" (or "at a time"). Update the text referencing EditorState in PROSEMIRROR-REFERENCE.md to remove "in time" so it reads concisely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/text-editor/prosemirror-adapter/plugins/table-plugin.spec.ts`:
- Around line 46-57: Extract the repeated "schema with table nodes" setup into a
small test helper: create a function (e.g., buildSchemaWithTables or
makeTestSchemaWithTables) that calls createCustomTestSchema({}), retrieves
getTableNodes(), merges those nodes into baseSchema.spec.nodes (using
nodes.addToEnd for each entry), and returns a new Schema constructed with the
combined nodes and baseSchema.spec.marks; then replace the duplicated blocks in
the tests (the sections that use baseSchema, tableNodes, nodes, and new Schema)
with calls to that helper (used in both the block at lines ~46-57 and the
similar block at ~90-97) so tests reuse the single helper.
In `@src/components/text-editor/PROSEMIRROR-REFERENCE.md`:
- Around line 691-693: Update the table entry that currently references
`plugins/table-plugin.ts` to use the consistent path
`prosemirror-adapter/plugins/table-plugin.ts` so it matches other references in
this document (e.g., the row listing custom NodeSpec and the surrounding
references); locate the table row containing `Custom NodeSpec` and
`plugins/table-plugin.ts` in PROSEMIRROR-REFERENCE.md and replace the path
string only.
In `@src/components/text-editor/test-setup/command-tester.ts`:
- Around line 29-57: Update getCommandResult to enforce that when any document
assertions (docContentAfter, docSizeAfter, includesContent) or shouldApply:true
are expected, a dispatch must have been called and transaction/newState must be
defined; if command returned true but transaction is undefined, return { result:
false } (or an error result) so tests don't falsely pass. Specifically, in
getCommandResult (and the analogous helpers around the other blocks referenced),
after running the command check: if any of
docContentAfter/docSizeAfter/includesContent or shouldApply is present and
transaction is undefined, treat this as a failure and do not skip verification —
require state.apply(transaction) to produce newState before performing document
assertions. Ensure the error path is explicit and includes the same result shape
(result:false) so callers can detect the missing dispatch.
In `@src/components/text-editor/test-setup/editor-view-builder.ts`:
- Around line 27-38: Wrap the supplied dispatchTransaction so the EditorView's
state is updated before delegating to the custom dispatcher: declare the view
variable so it can be referenced, set viewProps.dispatchTransaction to a
function that calls view.updateState(tr.state) (or assigns the new state) and
then calls the original dispatchTransaction(tr); this preserves the view.state
for tests while still invoking the provided spy/handler (refer to
dispatchTransaction, viewProps, EditorView and view in the diff).
In `@src/components/text-editor/test-setup/mock-factories.ts`:
- Around line 8-24: The JSDoc for createDispatchSpy is misleading because the
function requires an existing EditorView instance (view parameter) so it cannot
be passed into createEditorView; update the documentation for createDispatchSpy
to state clearly that it must be created after an EditorView exists, describe
that the spy wraps the provided view and optionally auto-applies transactions to
that view (autoUpdate), and remove the line claiming it can be passed as the
dispatchTransaction to createEditorView; alternatively, if you prefer a
different API, change createDispatchSpy to accept an optional view or return a
factory/attach function so callers can create the spy before or attach it after
creating the EditorView—reference createDispatchSpy, EditorView, and
createEditorView when making the change.
---
Outside diff comments:
In `@src/components/text-editor/prosemirror-adapter/plugins/table-plugin.ts`:
- Around line 15-18: The setDOMAttr function appends a new CSS declaration to
attrs.style without ensuring the existing style string ends with a semicolon,
which can produce malformed CSS; update setDOMAttr (the setDOMAttr: (value:
string, attrs: Record<string, any>) => { ... } block) to normalize attrs.style
before concatenation by trimming it, adding a trailing semicolon if missing,
then append `${cssProperty}: ${value};` only when value is truthy so the final
style string is always valid.
---
Duplicate comments:
In `@src/components/text-editor/PROSEMIRROR-REFERENCE.md`:
- Line 301: The sentence describing EditorState is wordy — change "EditorState
holds the complete state of the editor at a point in time" to a tighter phrasing
such as "EditorState holds the complete state of the editor at a point" (or "at
a time"). Update the text referencing EditorState in PROSEMIRROR-REFERENCE.md to
remove "in time" so it reads concisely.
In `@src/components/text-editor/test-setup/event-simulator.ts`:
- Around line 44-49: The helpers currently return the raw result of
domNode.dispatchEvent(event) (stored in eventHandled), which is false when
preventDefault() was called; instead return the handled-ness based on the
event's defaultPrevented flag. Replace usage of eventHandled/dispatchEvent()
return with reading event.defaultPrevented (i.e., return event.defaultPrevented)
after calling domNode.dispatchEvent(event); apply the same fix to the other
helper that dispatches the paste event (the second occurrence around lines
72-75) and keep view.dom and the dispatchEvent call intact.
In `@src/components/text-editor/TEXT-EDITOR-CONTEXT.md`:
- Around line 14-25: Add a language label (use "text") to each unlabeled fenced
block and ensure there is a blank line before and after each fenced block to
satisfy markdownlint rules (MD040/MD031); update the fenced blocks that contain
the limel-text-editor tree (including the block showing limel-notched-outline,
limel-prosemirror-adapter, `#editor`, .toolbar, limel-action-bar, limel-portal,
limel-text-editor-link-menu, limel-markdown, and limel-helper-line) as well as
the other similar unlabeled blocks mentioned, by replacing ``` with ```text and
making sure an empty line precedes and follows each fenced section.
🪄 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: 7b5bc45a-421c-4750-97fe-87f388c30b04
📒 Files selected for processing (15)
src/components/text-editor/PROSEMIRROR-REFERENCE.mdsrc/components/text-editor/TEXT-EDITOR-CONTEXT.mdsrc/components/text-editor/prosemirror-adapter/menu/menu-commands.tssrc/components/text-editor/prosemirror-adapter/plugins/table-plugin.spec.tssrc/components/text-editor/prosemirror-adapter/plugins/table-plugin.tssrc/components/text-editor/test-setup/command-tester.spec.tssrc/components/text-editor/test-setup/command-tester.tssrc/components/text-editor/test-setup/editor-test-utils.tssrc/components/text-editor/test-setup/editor-view-builder.spec.tssrc/components/text-editor/test-setup/editor-view-builder.tssrc/components/text-editor/test-setup/event-simulator.spec.tssrc/components/text-editor/test-setup/event-simulator.tssrc/components/text-editor/test-setup/mock-factories.spec.tssrc/components/text-editor/test-setup/mock-factories.tssrc/components/text-editor/test-setup/text-editor-test-suite-guide.md
src/components/text-editor/prosemirror-adapter/plugins/table-plugin.spec.ts
Show resolved
Hide resolved
john-traas
force-pushed
the
refine-text-editor-test-suite
branch
from
808b9c4 to
b88c2e0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/table-plugin.ts (1)
12-18:⚠️ Potential issue | 🟡 MinorNormalize existing
stylebefore appending.This now becomes reusable API, but direct concatenation produces invalid CSS when
attrs.styledoes not already end with;.Suggested fix
export const createStyleAttribute = (cssProperty: string) => ({ default: null, getFromDOM: (dom: HTMLElement) => dom.style[cssProperty] || null, setDOMAttr: (value: string, attrs: Record<string, any>) => { if (value) { - attrs.style = (attrs.style || '') + `${cssProperty}: ${value};`; + const existingStyle = + attrs.style && !attrs.style.trim().endsWith(';') + ? `${attrs.style};` + : attrs.style || ''; + + attrs.style = `${existingStyle}${cssProperty}: ${value};`; } }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/prosemirror-adapter/plugins/table-plugin.ts` around lines 12 - 18, The setDOMAttr implementation in createStyleAttribute appends a new CSS declaration by directly concatenating to attrs.style which can produce invalid CSS when attrs.style doesn't end with a semicolon; update setDOMAttr (inside createStyleAttribute) to normalize the existing attrs.style first (trim it, ensure it ends with a single semicolon and a space if non-empty) before appending `${cssProperty}: ${value};`, and consider replacing an existing declaration for the same cssProperty instead of duplicating it (parse attrs.style to remove any existing `${cssProperty}:...;` before appending).
♻️ Duplicate comments (7)
src/components/text-editor/PROSEMIRROR-REFERENCE.md (2)
692-692:⚠️ Potential issue | 🟡 MinorUse the consistent table-plugin path.
This row still says
plugins/table-plugin.ts, but the rest of the doc points toprosemirror-adapter/plugins/table-plugin.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/PROSEMIRROR-REFERENCE.md` at line 692, The table row listing "plugins/table-plugin.ts" is inconsistent with the rest of the doc; update that entry in PROSEMIRROR-REFERENCE.md (the "Custom `NodeSpec`" table row) to use the consistent path "prosemirror-adapter/plugins/table-plugin.ts" so it matches other references; ensure only the path text is changed and formatting of the table remains intact.
301-301:⚠️ Potential issue | 🟡 MinorTighten the redundant phrasing here.
“At a point in time” is repetitive. Prefer “at a point” or “at a time”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/PROSEMIRROR-REFERENCE.md` at line 301, The sentence describing EditorState is wordy; update the line that reads "EditorState holds the complete state of the editor at a point in time:" to a tighter phrasing such as "EditorState holds the complete state of the editor at a point." Keep the reference to EditorState unchanged and only replace the phrase "at a point in time" with "at a point" (or "at a time") to remove redundancy.src/components/text-editor/test-setup/event-simulator.ts (1)
44-49:⚠️ Potential issue | 🟠 MajorReturn
defaultPrevented, notdispatchEvent()’s boolean.
dispatchEvent()returnsfalsewhen a cancelable event is prevented, so both helpers currently report handled events asfalseand unhandled ones astrue.Suggested fix
const event = new KeyboardEvent('keydown', options); - - const domNode = view.dom; - const eventHandled = domNode.dispatchEvent(event); - - return eventHandled; + view.dom.dispatchEvent(event); + + return event.defaultPrevented; } @@ Object.defineProperty(pasteEvent, 'clipboardData', { value: dataTransfer, writable: false, }); - - const domNode = view.dom; - const eventHandled = domNode.dispatchEvent(pasteEvent); - - return eventHandled; + view.dom.dispatchEvent(pasteEvent); + + return pasteEvent.defaultPrevented; }What does `EventTarget.dispatchEvent()` return for a cancelable DOM event when a listener calls `preventDefault()`, and is `event.defaultPrevented` the correct way to detect that the event was handled?Also applies to: 72-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/test-setup/event-simulator.ts` around lines 44 - 49, The helpers use domNode.dispatchEvent(event) and return its boolean result (eventHandled), but dispatchEvent returns false when a cancelable event was prevented—so invert meaning; instead return event.defaultPrevented to indicate the event was handled. Update the code paths that create a KeyboardEvent (the variable event) and currently return eventHandled to return event.defaultPrevented; apply the same change to the other helper block that mirrors lines 72-75.src/components/text-editor/test-setup/editor-view-builder.ts (1)
27-38:⚠️ Potential issue | 🟠 MajorWrap the custom dispatcher so
view.statestays in sync.Passing the callback straight through breaks
EditorViewsemantics: the spy runs, butview.statenever advances afterview.dispatch(tr).Suggested fix
- const viewProps: { - state: EditorState; - dispatchTransaction?: (tr: Transaction) => void; - } = { - state: editorState, - }; - - if (dispatchTransaction) { - viewProps.dispatchTransaction = dispatchTransaction; - } - - const view = new EditorView(container, viewProps); + let view: EditorView; + + view = new EditorView(container, { + state: editorState, + dispatchTransaction: (tr) => { + view.updateState(view.state.apply(tr)); + dispatchTransaction?.(tr); + }, + });In ProseMirror's EditorView, when a custom `dispatchTransaction` prop is provided, is that callback responsible for applying `tr` to `view.state` and calling `view.updateState(newState)`?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/test-setup/editor-view-builder.ts` around lines 27 - 38, The provided dispatchTransaction is passed straight into EditorView (via viewProps.dispatchTransaction) which prevents view.state from advancing; wrap the incoming dispatchTransaction so that when a transaction is dispatched you first compute const newState = view.state.apply(tr), call view.updateState(newState) (or view.updateState(view.state.apply(tr))) to keep the view in sync, and then invoke the original dispatchTransaction callback (if any) with the transaction and the view; update the construction around viewProps/EditorView to use this wrapper so view.dispatch(tr) behaves correctly while still calling the test spy.src/components/text-editor/test-setup/mock-factories.ts (1)
8-24:⚠️ Potential issue | 🟡 MinorFix the circular setup described in the JSDoc.
createDispatchSpy()needs an existingview, so the returned spy cannot be created first and then passed intocreateEditorView(...). Reword the contract to say it wraps an already-created view.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/test-setup/mock-factories.ts` around lines 8 - 24, The JSDoc for createDispatchSpy is misleading about setup order; update the comment to state clearly that createDispatchSpy requires an already-created EditorView and wraps that existing view's dispatchTransaction, so the spy must be created after the view (i.e., you cannot create the spy first and pass it into createEditorView). Modify the `@param` and summary to reflect that createDispatchSpy wraps an existing view's dispatch behavior (reference: createDispatchSpy and createEditorView) and clarify the usage pattern and parameters accordingly.src/components/text-editor/test-setup/command-tester.ts (1)
162-169:⚠️ Potential issue | 🟠 MajorRequire a dispatched state before validating post-command document expectations.
Lines 168-169 and 226-227 still silently skip document assertions when a command returns
truebut never dispatches. That makesdocContentAfter,docSizeAfter, andincludesContentfalse-green for no-op commands.🧩 Suggested fix
export function testCommand( command: Command, state: EditorState, expected: { shouldApply: boolean; docContentAfter?: string; docSizeAfter?: number; includesContent?: string | string[]; } ): CommandResult { const commandResult = getCommandResult(command, state); + const hasDocumentAssertions = + expected.docContentAfter !== undefined || + expected.docSizeAfter !== undefined || + expected.includesContent !== undefined; // Verify if command was applicable as expected expect(commandResult.result).toBe(expected.shouldApply); - // If command should apply, verify the new state if expectations are provided - if (expected.shouldApply && commandResult.newState) { - verifyDocumentContent(commandResult.newState, expected); + if (expected.shouldApply && hasDocumentAssertions) { + expect(commandResult.newState).toBeDefined(); + verifyDocumentContent(commandResult.newState!, expected); } return commandResult; } @@ if (transaction) { commandResultObj.transaction = transaction; commandResultObj.newState = state.apply(transaction); } + const hasDocumentAssertions = + expected.docContentAfter !== undefined || + expected.docSizeAfter !== undefined || + expected.includesContent !== undefined; + expect(commandResultObj.result).toBe(expected.shouldApply); - if (expected.shouldApply && commandResultObj.newState) { - verifyDocumentContent(commandResultObj.newState, expected); + if (expected.shouldApply && hasDocumentAssertions) { + expect(commandResultObj.newState).toBeDefined(); + verifyDocumentContent(commandResultObj.newState!, expected); }Also applies to: 224-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/test-setup/command-tester.ts` around lines 162 - 169, The test currently skips document assertions when a command reports it applied but never produced a dispatched state; update the assertion logic so that when expected.shouldApply is true you first assert that commandResult.newState (or the dispatched state returned by getCommandResult) is defined/was dispatched, then call verifyDocumentContent(commandResult.newState, expected); likewise ensure when expected.shouldApply is false there is no newState—use explicit expects around commandResult.newState and replace the current if (expected.shouldApply && commandResult.newState) guard with an explicit presence check before calling verifyDocumentContent.src/components/text-editor/TEXT-EDITOR-CONTEXT.md (1)
3-3:⚠️ Potential issue | 🟡 MinorAddress the markdownlint violations before merging.
Static analysis is already reporting MD022/MD031/MD040/MD058 here: headings need blank lines around them, fences need language tags and surrounding blank lines, and tables should also be surrounded by blank lines. The unlabeled fenced blocks were already called out previously and are still present.
Also applies to: 14-14, 67-67, 81-81, 86-86, 125-125, 148-148, 176-176, 217-217
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/TEXT-EDITOR-CONTEXT.md` at line 3, The file contains markdownlint violations (MD022/MD031/MD040/MD058) caused by headings and fenced blocks lacking surrounding blank lines and unlabeled code fences; fix by ensuring every heading like "## Overview" and other headings listed (lines referenced in review) has a blank line before and after, add language tags to all fenced code blocks and surround them with blank lines, and ensure any tables are separated from surrounding text with blank lines; run markdownlint to verify no MD022/MD031/MD040/MD058 remain before merging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/text-editor/PROSEMIRROR-REFERENCE.md`:
- Line 14: Several sections (e.g., the heading "The fundamental design
principle" and other nearby section headings) violate markdownlint rules
MD022/MD031/MD040; run markdownlint, then fix by ensuring blank lines surround
all headings (MD022), ensure fenced code blocks are surrounded by blank lines
(MD031), and add language identifiers to fenced code blocks or remove stray
fences as appropriate (MD040); scan the file for similar patterns in the other
flagged sections and apply the same fixes consistently so the lint output is
clean.
In `@src/components/text-editor/test-setup/editor-view-builder.spec.ts`:
- Around line 41-50: The test currently only asserts the dispatchSpy was called;
also assert the EditorView's state actually updated after dispatch to catch
stale-state regressions: after creating the view via createEditorView(...) and
calling view.dispatch(tr), add an assertion that the view.state reflects the
inserted text or transaction (e.g. verify view.state.doc/textContent or
equivalent API contains "Test" or that the selection/step count changed) so both
dispatchSpy and view.state are validated.
In `@src/components/text-editor/test-setup/text-editor-test-suite-guide.md`:
- Around line 54-65: Update the docs to match the actual helper signature:
change the documented signature from createDispatchSpy(autoUpdate?) to
createDispatchSpy(view, autoUpdate?) and update the example to pass an
EditorView instance (e.g., the view returned from createEditorView) as the first
argument when calling createDispatchSpy; reference createDispatchSpy and the
sample usage alongside createEditorView so readers pass the view into
createDispatchSpy instead of only a boolean.
---
Outside diff comments:
In `@src/components/text-editor/prosemirror-adapter/plugins/table-plugin.ts`:
- Around line 12-18: The setDOMAttr implementation in createStyleAttribute
appends a new CSS declaration by directly concatenating to attrs.style which can
produce invalid CSS when attrs.style doesn't end with a semicolon; update
setDOMAttr (inside createStyleAttribute) to normalize the existing attrs.style
first (trim it, ensure it ends with a single semicolon and a space if non-empty)
before appending `${cssProperty}: ${value};`, and consider replacing an existing
declaration for the same cssProperty instead of duplicating it (parse
attrs.style to remove any existing `${cssProperty}:...;` before appending).
---
Duplicate comments:
In `@src/components/text-editor/PROSEMIRROR-REFERENCE.md`:
- Line 692: The table row listing "plugins/table-plugin.ts" is inconsistent with
the rest of the doc; update that entry in PROSEMIRROR-REFERENCE.md (the "Custom
`NodeSpec`" table row) to use the consistent path
"prosemirror-adapter/plugins/table-plugin.ts" so it matches other references;
ensure only the path text is changed and formatting of the table remains intact.
- Line 301: The sentence describing EditorState is wordy; update the line that
reads "EditorState holds the complete state of the editor at a point in time:"
to a tighter phrasing such as "EditorState holds the complete state of the
editor at a point." Keep the reference to EditorState unchanged and only replace
the phrase "at a point in time" with "at a point" (or "at a time") to remove
redundancy.
In `@src/components/text-editor/test-setup/command-tester.ts`:
- Around line 162-169: The test currently skips document assertions when a
command reports it applied but never produced a dispatched state; update the
assertion logic so that when expected.shouldApply is true you first assert that
commandResult.newState (or the dispatched state returned by getCommandResult) is
defined/was dispatched, then call verifyDocumentContent(commandResult.newState,
expected); likewise ensure when expected.shouldApply is false there is no
newState—use explicit expects around commandResult.newState and replace the
current if (expected.shouldApply && commandResult.newState) guard with an
explicit presence check before calling verifyDocumentContent.
In `@src/components/text-editor/test-setup/editor-view-builder.ts`:
- Around line 27-38: The provided dispatchTransaction is passed straight into
EditorView (via viewProps.dispatchTransaction) which prevents view.state from
advancing; wrap the incoming dispatchTransaction so that when a transaction is
dispatched you first compute const newState = view.state.apply(tr), call
view.updateState(newState) (or view.updateState(view.state.apply(tr))) to keep
the view in sync, and then invoke the original dispatchTransaction callback (if
any) with the transaction and the view; update the construction around
viewProps/EditorView to use this wrapper so view.dispatch(tr) behaves correctly
while still calling the test spy.
In `@src/components/text-editor/test-setup/event-simulator.ts`:
- Around line 44-49: The helpers use domNode.dispatchEvent(event) and return its
boolean result (eventHandled), but dispatchEvent returns false when a cancelable
event was prevented—so invert meaning; instead return event.defaultPrevented to
indicate the event was handled. Update the code paths that create a
KeyboardEvent (the variable event) and currently return eventHandled to return
event.defaultPrevented; apply the same change to the other helper block that
mirrors lines 72-75.
In `@src/components/text-editor/test-setup/mock-factories.ts`:
- Around line 8-24: The JSDoc for createDispatchSpy is misleading about setup
order; update the comment to state clearly that createDispatchSpy requires an
already-created EditorView and wraps that existing view's dispatchTransaction,
so the spy must be created after the view (i.e., you cannot create the spy first
and pass it into createEditorView). Modify the `@param` and summary to reflect
that createDispatchSpy wraps an existing view's dispatch behavior (reference:
createDispatchSpy and createEditorView) and clarify the usage pattern and
parameters accordingly.
In `@src/components/text-editor/TEXT-EDITOR-CONTEXT.md`:
- Line 3: The file contains markdownlint violations (MD022/MD031/MD040/MD058)
caused by headings and fenced blocks lacking surrounding blank lines and
unlabeled code fences; fix by ensuring every heading like "## Overview" and
other headings listed (lines referenced in review) has a blank line before and
after, add language tags to all fenced code blocks and surround them with blank
lines, and ensure any tables are separated from surrounding text with blank
lines; run markdownlint to verify no MD022/MD031/MD040/MD058 remain before
merging.
🪄 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: 643ebe09-1b0b-457a-b1b6-9c3ad391405b
📒 Files selected for processing (15)
src/components/text-editor/PROSEMIRROR-REFERENCE.mdsrc/components/text-editor/TEXT-EDITOR-CONTEXT.mdsrc/components/text-editor/prosemirror-adapter/menu/menu-commands.tssrc/components/text-editor/prosemirror-adapter/plugins/table-plugin.spec.tssrc/components/text-editor/prosemirror-adapter/plugins/table-plugin.tssrc/components/text-editor/test-setup/command-tester.spec.tssrc/components/text-editor/test-setup/command-tester.tssrc/components/text-editor/test-setup/editor-test-utils.tssrc/components/text-editor/test-setup/editor-view-builder.spec.tssrc/components/text-editor/test-setup/editor-view-builder.tssrc/components/text-editor/test-setup/event-simulator.spec.tssrc/components/text-editor/test-setup/event-simulator.tssrc/components/text-editor/test-setup/mock-factories.spec.tssrc/components/text-editor/test-setup/mock-factories.tssrc/components/text-editor/test-setup/text-editor-test-suite-guide.md
src/components/text-editor/test-setup/text-editor-test-suite-guide.md
Outdated
Show resolved
Hide resolved
john-traas
force-pushed
the
refine-text-editor-test-suite
branch
from
b88c2e0 to
463b3b0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
src/components/text-editor/test-setup/editor-view-builder.spec.ts (1)
41-51:⚠️ Potential issue | 🟡 MinorAlso assert state mutation in the custom-dispatch test.
This case currently proves callback invocation only; it should also verify that
view.statechanged after dispatch to catch stale-state regressions.💡 Suggested test addition
it('should use provided dispatch function', () => { @@ const tr = view.state.tr.insertText('Test'); view.dispatch(tr); expect(dispatchSpy).toHaveBeenCalledWith(tr); + expect(view.state.doc.textContent).toBe('Test'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/test-setup/editor-view-builder.spec.ts` around lines 41 - 51, The test for createEditorView's custom dispatch currently only checks that dispatchSpy was called; update it to also assert that the editor state actually mutated after dispatch to catch stale-state regressions by capturing the state (or doc text) before dispatch (e.g., prevDoc = view.state.doc or prevText = view.state.doc.textBetween(...)), performing view.dispatch(tr) as done now, and then asserting that view.state (or view.state.doc/text) differs from the previous snapshot and contains the inserted "Test"; keep references to createEditorView, view, dispatchSpy, and tr so the assertion is placed in the same 'should use provided dispatch function' test.src/components/text-editor/test-setup/editor-view-builder.ts (1)
34-39:⚠️ Potential issue | 🟠 MajorWrap custom dispatch so
EditorViewstate is always updated.When a custom dispatcher is provided, Line 34-35 bypasses the normal apply/update flow, so
view.statecan become stale afterview.dispatch(tr).💡 Suggested fix
export function createEditorView( state?: EditorState, dispatchTransaction?: (tr: Transaction) => void, parentElement?: HTMLElement ): { view: EditorView; container: HTMLElement } { @@ - const viewProps: { - state: EditorState; - dispatchTransaction?: (tr: Transaction) => void; - } = { - state: editorState, - }; - - if (dispatchTransaction) { - viewProps.dispatchTransaction = dispatchTransaction; - } - - const view = new EditorView(container, viewProps); + let view: EditorView; + view = new EditorView(container, { + state: editorState, + dispatchTransaction: (tr: Transaction) => { + const nextState = view.state.apply(tr); + view.updateState(nextState); + dispatchTransaction?.(tr); + }, + });#!/bin/bash # Verify dispatchTransaction implementations in text-editor update view state. rg -nP --type=ts --type=tsx "dispatchTransaction:\s*\(tr\)\s*=>\s*\{" src/components/text-editor -C4🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/test-setup/editor-view-builder.ts` around lines 34 - 39, The custom dispatchTransaction passed into viewProps should be wrapped so the EditorView's state is updated before calling the user's handler: replace direct assignment of viewProps.dispatchTransaction = dispatchTransaction with a wrapper that computes the new state via view.state.apply(tr) and calls view.updateState(newState) (using the EditorView instance created as view) and only then invokes the provided dispatchTransaction(tr); this ensures view.state stays in sync after view.dispatch(tr) while preserving the custom handler behavior.src/components/text-editor/test-setup/event-simulator.ts (2)
44-49:⚠️ Potential issue | 🟠 MajorReturn value is inverted for “handled” semantics.
dispatchEvent()returnsfalsewhenpreventDefault()is called on a cancelable event, so these helpers currently report handled events asfalse.💡 Suggested fix
const event = new KeyboardEvent('keydown', options); - - const domNode = view.dom; - const eventHandled = domNode.dispatchEvent(event); - - return eventHandled; + view.dom.dispatchEvent(event); + return event.defaultPrevented; } @@ - const domNode = view.dom; - const eventHandled = domNode.dispatchEvent(pasteEvent); - - return eventHandled; + view.dom.dispatchEvent(pasteEvent); + return pasteEvent.defaultPrevented; }What does EventTarget.dispatchEvent() return for a cancelable event when a listener calls preventDefault(), and should "handled" be represented by event.defaultPrevented instead?Also applies to: 72-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/test-setup/event-simulator.ts` around lines 44 - 49, The helper currently treats the boolean returned by domNode.dispatchEvent(event) as "handled", but dispatchEvent returns false when preventDefault() was called on a cancelable event (i.e., handled), so invert the logic: use event.defaultPrevented to represent "handled" (or negate dispatchEvent result) and update both occurrences (the local variable eventHandled and the return at the later block around lines 72-75) to return true when event.defaultPrevented is true.
85-85:⚠️ Potential issue | 🟡 MinorMark immutable field as
readonly.
ClipboardDataMock.datais never reassigned and can be declared readonly.💡 Suggested cleanup
- private data = new Map<string, string>(); + private readonly data = new Map<string, string>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/test-setup/event-simulator.ts` at line 85, The ClipboardDataMock class has a field data that is never reassigned; change its declaration to a readonly property (e.g., private readonly data: Map<string, string>) so the field is immutable, update any references that mutate the map itself are allowed but ensure you do not reassign data anywhere in methods like getData/ setData so the code compiles and behavior remains the same.src/components/text-editor/test-setup/command-tester.ts (1)
235-239:⚠️ Potential issue | 🟠 Major
testCommandWithViewcan still false-green on missing dispatch.At Lines 237-239, document assertions are skipped when
newStateis undefined, even if expectations were provided. Mirror the stricter guard used intestCommand.Proposed fix
expect(commandResultObj.result).toBe(expected.shouldApply); - if (expected.shouldApply && commandResultObj.newState) { - verifyDocumentContent(commandResultObj.newState, expected); + if (expected.shouldApply) { + const hasDocExpectations = + expected.docContentAfter !== undefined || + expected.docSizeAfter !== undefined || + expected.includesContent !== undefined; + + if (hasDocExpectations) { + expect(commandResultObj.newState).toBeDefined(); + } + + if (commandResultObj.newState) { + verifyDocumentContent(commandResultObj.newState, expected); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/test-setup/command-tester.ts` around lines 235 - 239, The current testCommandWithView allows false-green results by skipping document assertions when commandResultObj.newState is undefined; update testCommandWithView to mirror testCommand's stricter guard: when expected.shouldApply is true, assert that commandResultObj.newState is defined (fail the test if not) before calling verifyDocumentContent, then call verifyDocumentContent(commandResultObj.newState, expected). Reference the testCommandWithView function, commandResultObj and verifyDocumentContent to locate and apply this change.src/components/text-editor/test-setup/content-generator.ts (1)
24-31:⚠️ Potential issue | 🟠 Major
createDocWithTextcurrently parses markup instead of preserving literal text.At Line 29, interpolating into HTML (
<p>${text}</p>) means input like<strong>x</strong>is treated as markup, not plain text. Build this doc from schema nodes instead.Proposed fix
export function createDocWithText( text: string = '', schema?: Schema ): EditorState { const editorSchema = schema || createTestSchema(); - const content = text ? `<p>${text}</p>` : '<p></p>'; - - return createEditorState(content, editorSchema); + const paragraph = editorSchema.nodes.paragraph.create( + null, + text ? editorSchema.text(text) : undefined + ); + const doc = editorSchema.nodes.doc.create(null, paragraph); + + return EditorState.create({ doc }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/test-setup/content-generator.ts` around lines 24 - 31, createDocWithText currently interpolates the input into HTML (`<p>${text}</p>`) which causes markup to be parsed; instead build the document using the schema nodes so text is preserved literally: in createDocWithText, use the computed editorSchema (from createTestSchema when schema is undefined) to create a paragraph node containing a text node from editorSchema.text(...) (or editorSchema.nodes.text) and then build the top-level doc node (editorSchema.nodes.doc/your root node) from that paragraph before calling createEditorState; update references in the function to use the schema node constructors rather than an HTML string so inputs like "<strong>x</strong>" remain literal.src/components/text-editor/test-setup/content-generator.spec.ts (1)
179-185:⚠️ Potential issue | 🟡 MinorAvoid vacuous pass in default heading-level test.
At Line 183, the
if (firstChild)guard can let the test pass without validating anything. Add an explicit existence assertion first, then assertlevel.Proposed fix
it('should default to level 1 if not specified', () => { const state = createDocWithHeading('Heading'); const firstChild = state.doc.firstChild; - if (firstChild) { - expect(firstChild.attrs.level).toBe(1); - } + expect(firstChild).toBeDefined(); + expect(firstChild!.attrs.level).toBe(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/test-setup/content-generator.spec.ts` around lines 179 - 185, The test "should default to level 1 if not specified" uses an if (firstChild) guard which can make the test vacuously pass; change it to assert the node exists then assert its level: replace the conditional with an explicit existence assertion on state.doc.firstChild (e.g., expect(firstChild).toBeDefined() or not.toBeNull()) and then assert expect(firstChild.attrs.level).toBe(1); keep createDocWithHeading and the same variables but remove the early-returning conditional so the test fails if firstChild is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/text-editor/prosemirror-adapter/menu/menu-commands.ts`:
- Around line 152-163: The current unwrap uses a generic lift() that may lift
the wrong ancestor; instead, when checkForActiveWrap (or the toggle-wrap logic
that iterates $from.depth and matches nodeType) finds the specific ancestor
depth d, compute that ancestor's exact document range using $from.start(d) and
$from.end(d), determine the proper lift target for that range (using
prosemirror-transform's liftTarget on the slice/fragment) and call
tr.lift(range, target) or apply the lift command specifically to that range
rather than calling the generic lift() command; update the code paths in the
toggle/wrap removal logic that currently call lift() so they compute start/end
from the matched depth and perform a targeted lift on that range.
In `@src/components/text-editor/test-setup/event-simulator.spec.ts`:
- Around line 39-40: The test calls simulateKeyPress(view, 'b', { ctrl: true })
but doesn't assert its boolean "handled" return value; update the test to
capture the return value from simulateKeyPress (and the other similar call
around lines 96-100) and add an explicit assertion (e.g.,
expect(handled).toBe(true) or toBe(false) according to the intended contract) to
lock the handled-by-editor contract and catch regressions.
---
Duplicate comments:
In `@src/components/text-editor/test-setup/command-tester.ts`:
- Around line 235-239: The current testCommandWithView allows false-green
results by skipping document assertions when commandResultObj.newState is
undefined; update testCommandWithView to mirror testCommand's stricter guard:
when expected.shouldApply is true, assert that commandResultObj.newState is
defined (fail the test if not) before calling verifyDocumentContent, then call
verifyDocumentContent(commandResultObj.newState, expected). Reference the
testCommandWithView function, commandResultObj and verifyDocumentContent to
locate and apply this change.
In `@src/components/text-editor/test-setup/content-generator.spec.ts`:
- Around line 179-185: The test "should default to level 1 if not specified"
uses an if (firstChild) guard which can make the test vacuously pass; change it
to assert the node exists then assert its level: replace the conditional with an
explicit existence assertion on state.doc.firstChild (e.g.,
expect(firstChild).toBeDefined() or not.toBeNull()) and then assert
expect(firstChild.attrs.level).toBe(1); keep createDocWithHeading and the same
variables but remove the early-returning conditional so the test fails if
firstChild is missing.
In `@src/components/text-editor/test-setup/content-generator.ts`:
- Around line 24-31: createDocWithText currently interpolates the input into
HTML (`<p>${text}</p>`) which causes markup to be parsed; instead build the
document using the schema nodes so text is preserved literally: in
createDocWithText, use the computed editorSchema (from createTestSchema when
schema is undefined) to create a paragraph node containing a text node from
editorSchema.text(...) (or editorSchema.nodes.text) and then build the top-level
doc node (editorSchema.nodes.doc/your root node) from that paragraph before
calling createEditorState; update references in the function to use the schema
node constructors rather than an HTML string so inputs like "<strong>x</strong>"
remain literal.
In `@src/components/text-editor/test-setup/editor-view-builder.spec.ts`:
- Around line 41-51: The test for createEditorView's custom dispatch currently
only checks that dispatchSpy was called; update it to also assert that the
editor state actually mutated after dispatch to catch stale-state regressions by
capturing the state (or doc text) before dispatch (e.g., prevDoc =
view.state.doc or prevText = view.state.doc.textBetween(...)), performing
view.dispatch(tr) as done now, and then asserting that view.state (or
view.state.doc/text) differs from the previous snapshot and contains the
inserted "Test"; keep references to createEditorView, view, dispatchSpy, and tr
so the assertion is placed in the same 'should use provided dispatch function'
test.
In `@src/components/text-editor/test-setup/editor-view-builder.ts`:
- Around line 34-39: The custom dispatchTransaction passed into viewProps should
be wrapped so the EditorView's state is updated before calling the user's
handler: replace direct assignment of viewProps.dispatchTransaction =
dispatchTransaction with a wrapper that computes the new state via
view.state.apply(tr) and calls view.updateState(newState) (using the EditorView
instance created as view) and only then invokes the provided
dispatchTransaction(tr); this ensures view.state stays in sync after
view.dispatch(tr) while preserving the custom handler behavior.
In `@src/components/text-editor/test-setup/event-simulator.ts`:
- Around line 44-49: The helper currently treats the boolean returned by
domNode.dispatchEvent(event) as "handled", but dispatchEvent returns false when
preventDefault() was called on a cancelable event (i.e., handled), so invert the
logic: use event.defaultPrevented to represent "handled" (or negate
dispatchEvent result) and update both occurrences (the local variable
eventHandled and the return at the later block around lines 72-75) to return
true when event.defaultPrevented is true.
- Line 85: The ClipboardDataMock class has a field data that is never
reassigned; change its declaration to a readonly property (e.g., private
readonly data: Map<string, string>) so the field is immutable, update any
references that mutate the map itself are allowed but ensure you do not reassign
data anywhere in methods like getData/ setData so the code compiles and behavior
remains the same.
🪄 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: 1ebb7f4d-e4d5-43bf-ab84-aa6f573a3dbc
📒 Files selected for processing (15)
src/components/text-editor/prosemirror-adapter/menu/menu-commands.tssrc/components/text-editor/prosemirror-adapter/plugins/table-plugin.spec.tssrc/components/text-editor/prosemirror-adapter/plugins/table-plugin.tssrc/components/text-editor/test-setup/command-tester.spec.tssrc/components/text-editor/test-setup/command-tester.tssrc/components/text-editor/test-setup/content-generator.spec.tssrc/components/text-editor/test-setup/content-generator.tssrc/components/text-editor/test-setup/editor-test-utils.tssrc/components/text-editor/test-setup/editor-view-builder.spec.tssrc/components/text-editor/test-setup/editor-view-builder.tssrc/components/text-editor/test-setup/event-simulator.spec.tssrc/components/text-editor/test-setup/event-simulator.tssrc/components/text-editor/test-setup/mock-factories.spec.tssrc/components/text-editor/test-setup/mock-factories.tssrc/components/text-editor/test-setup/text-editor-test-suite-guide.md
john-traas
force-pushed
the
refine-text-editor-test-suite
branch
from
463b3b0 to
30f008e
Compare
Contributor
There was a problem hiding this comment.
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/table-plugin.ts (1)
12-19:⚠️ Potential issue | 🟠 MajorBroaden
setDOMAttrinput typing to match actual usage.
setDOMAttris typed asvalue: string, but the function is called withnull(line 158 of the test) andundefined(line 162 of the test), with intentional guards already in place to handle these cases. The current signature does not reflect actual usage.Proposed fix
+type TableCellStyleAttrs = { style?: string }; + export const createStyleAttribute = (cssProperty: string) => ({ default: null, getFromDOM: (dom: HTMLElement) => dom.style[cssProperty] || null, - setDOMAttr: (value: string, attrs: Record<string, any>) => { + setDOMAttr: ( + value: string | null | undefined, + attrs: TableCellStyleAttrs + ) => { if (value) { attrs.style = (attrs.style || '') + `${cssProperty}: ${value};`; } }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/prosemirror-adapter/plugins/table-plugin.ts` around lines 12 - 19, The setDOMAttr parameter typing in createStyleAttribute is too narrow: update the signature of setDOMAttr on createStyleAttribute to accept value: string | null | undefined (or a broader type like any/unknown) instead of only string so callers that pass null/undefined (as in tests) are correctly typed; leave the existing null/undefined guards in getFromDOM/setDOMAttr unchanged and only adjust the type for setDOMAttr in the object returned by createStyleAttribute to reflect actual usage.
♻️ Duplicate comments (3)
src/components/text-editor/test-setup/event-simulator.spec.ts (1)
39-40:⚠️ Potential issue | 🟡 MinorAssert the helper boolean return contract explicitly.
These tests verify document mutations, but not the returned handled flag. Please assert the return values to lock API behavior.
✅ Suggested test additions
- simulateKeyPress(view, 'b', { ctrl: true }); + const keyHandled = simulateKeyPress(view, 'b', { ctrl: true }); + expect(keyHandled).toBe(true); @@ - simulatePaste(view, { text: 'pasted content' }); + const pasteHandled = simulatePaste(view, { + text: 'pasted content', + }); + expect(pasteHandled).toBe(true);Also applies to: 96-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/test-setup/event-simulator.spec.ts` around lines 39 - 40, Tests calling the helper simulateKeyPress currently assert document mutations but not the boolean "handled" return; update the calls in event-simulator.spec.ts (the occurrences using simulateKeyPress, e.g., the call with 'b' and the one at lines ~96-97) to capture its return value and add assertions like expect(handled).toBe(true) or expect(handled).toBe(false) that match the intended behavior for each scenario, ensuring the tests explicitly lock the helper's handled flag contract.src/components/text-editor/test-setup/event-simulator.ts (2)
27-50:⚠️ Potential issue | 🟠 MajorReturn-value contract is inverted vs documented “handled” semantics.
Both helpers currently return raw
dispatchEvent(...), which isfalsewhen a listener prevents default. That conflicts with “handled by the editor” wording and can mislead callers.💡 Proposed fix (align implementation with documented contract)
- * `@returns` Whether the key event was handled by the editor + * `@returns` Whether the key event was handled by the editor @@ - const domNode = view.dom; - const eventHandled = domNode.dispatchEvent(event); - - return eventHandled; + view.dom.dispatchEvent(event); + return event.defaultPrevented; @@ - * `@returns` Whether the paste event was handled by the editor + * `@returns` Whether the paste event was handled by the editor @@ - const domNode = view.dom; - const eventHandled = domNode.dispatchEvent(pasteEvent); - - return eventHandled; + view.dom.dispatchEvent(pasteEvent); + return pasteEvent.defaultPrevented;What does EventTarget.dispatchEvent() return for a cancelable event when a listener calls preventDefault(), according to MDN or the DOM specification?Also applies to: 57-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/test-setup/event-simulator.ts` around lines 27 - 50, The function simulateKeyPress (and the sibling helper at lines 57-76) currently returns the raw dispatchEvent result (false when preventDefault() was called), which is opposite the documented "handled by the editor" semantics; change both helpers to return the inverse of dispatchEvent's return (e.g., return !domNode.dispatchEvent(event)) so that a prevented default yields true (handled) and non-prevented yields false (not handled), keeping all existing event construction and dispatch logic unchanged.
110-116:⚠️ Potential issue | 🟡 MinorExplicit empty clipboard values are currently dropped.
Using truthy checks means
''is ignored. Prefer!== undefinedso tests can simulate intentionally emptytext/plainortext/html.🩹 Proposed fix
- if (content.text) { + if (content.text !== undefined) { dataTransfer.setData('text/plain', content.text); } - if (content.html) { + if (content.html !== undefined) { dataTransfer.setData('text/html', content.html); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/text-editor/test-setup/event-simulator.ts` around lines 110 - 116, The checks that set clipboard data drop intentionally empty strings because they use truthy checks; update the conditions around dataTransfer.setData calls in event-simulator.ts to check for undefined instead (e.g., use content.text !== undefined and content.html !== undefined) so that empty string values for content.text or content.html are preserved when calling dataTransfer.setData('text/plain', ...) and dataTransfer.setData('text/html', ...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/components/text-editor/prosemirror-adapter/plugins/table-plugin.ts`:
- Around line 12-19: The setDOMAttr parameter typing in createStyleAttribute is
too narrow: update the signature of setDOMAttr on createStyleAttribute to accept
value: string | null | undefined (or a broader type like any/unknown) instead of
only string so callers that pass null/undefined (as in tests) are correctly
typed; leave the existing null/undefined guards in getFromDOM/setDOMAttr
unchanged and only adjust the type for setDOMAttr in the object returned by
createStyleAttribute to reflect actual usage.
---
Duplicate comments:
In `@src/components/text-editor/test-setup/event-simulator.spec.ts`:
- Around line 39-40: Tests calling the helper simulateKeyPress currently assert
document mutations but not the boolean "handled" return; update the calls in
event-simulator.spec.ts (the occurrences using simulateKeyPress, e.g., the call
with 'b' and the one at lines ~96-97) to capture its return value and add
assertions like expect(handled).toBe(true) or expect(handled).toBe(false) that
match the intended behavior for each scenario, ensuring the tests explicitly
lock the helper's handled flag contract.
In `@src/components/text-editor/test-setup/event-simulator.ts`:
- Around line 27-50: The function simulateKeyPress (and the sibling helper at
lines 57-76) currently returns the raw dispatchEvent result (false when
preventDefault() was called), which is opposite the documented "handled by the
editor" semantics; change both helpers to return the inverse of dispatchEvent's
return (e.g., return !domNode.dispatchEvent(event)) so that a prevented default
yields true (handled) and non-prevented yields false (not handled), keeping all
existing event construction and dispatch logic unchanged.
- Around line 110-116: The checks that set clipboard data drop intentionally
empty strings because they use truthy checks; update the conditions around
dataTransfer.setData calls in event-simulator.ts to check for undefined instead
(e.g., use content.text !== undefined and content.html !== undefined) so that
empty string values for content.text or content.html are preserved when calling
dataTransfer.setData('text/plain', ...) and dataTransfer.setData('text/html',
...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 590255f6-3fdf-4bbf-b8f0-96d6639a85fe
📒 Files selected for processing (8)
src/components/text-editor/prosemirror-adapter/plugins/table-plugin.spec.tssrc/components/text-editor/prosemirror-adapter/plugins/table-plugin.tssrc/components/text-editor/test-setup/editor-test-utils.tssrc/components/text-editor/test-setup/event-simulator.spec.tssrc/components/text-editor/test-setup/event-simulator.tssrc/components/text-editor/test-setup/mock-factories.spec.tssrc/components/text-editor/test-setup/mock-factories.tssrc/components/text-editor/test-setup/text-editor-test-suite-guide.md
john-traas
force-pushed
the
refine-text-editor-test-suite
branch
from
30f008e to
e72eae5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/text-editor/prosemirror-adapter/plugins/table-plugin.spec.ts`:
- Around line 18-19: The test-scoped variables view and container in
table-plugin.spec.ts are declared as possibly null but left uninitialized
(undefined at runtime); explicitly initialize them to null by updating the
declarations for view and container so their runtime value matches the declared
type and the afterEach() cleanup that resets them to null (look for the let
view: EditorView | null and let container: HTMLElement | null declarations and
set each to = null).
In `@src/components/text-editor/prosemirror-adapter/plugins/table-plugin.ts`:
- Around line 16-17: The code appends a new CSS declaration to attrs.style
without ensuring a trailing separator, which can produce invalid CSS when
attrs.style doesn't end with a semicolon; update the logic in the table-plugin's
style handling (the block that touches attrs.style, cssProperty and value) to
normalize attrs.style first—trim it, if it's non-empty ensure it ends with a
semicolon (or add one and a space) before concatenating the new `${cssProperty}:
${value};` declaration so declarations are always properly separated.
🪄 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: 1e49f47d-9ad3-4b82-a514-8aebbf978acd
📒 Files selected for processing (2)
src/components/text-editor/prosemirror-adapter/plugins/table-plugin.spec.tssrc/components/text-editor/prosemirror-adapter/plugins/table-plugin.ts
src/components/text-editor/prosemirror-adapter/plugins/table-plugin.spec.ts
Outdated
Show resolved
Hide resolved
Add test-side mock utilities that depend on vitest: createDispatchSpy and createMockEditorView. These live separately from editor-view-builder so that the infrastructure module stays free of test framework imports. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
john-traas
force-pushed
the
refine-text-editor-test-suite
branch
from
e72eae5 to
1c5745c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/text-editor/test-setup/event-simulator.spec.ts`:
- Around line 41-46: The test reads textNode.marks which can throw if textNode
is undefined; after obtaining textNode (from
view.state.doc.firstChild?.firstChild) add an explicit guard/assertion before
accessing marks (e.g., keep or strengthen expect(textNode).toBeDefined() and
only compute hasStrongMark after that assertion) so the failure message is
clear; update the code that computes hasStrongMark (referencing textNode and
hasStrongMark) to occur after the guard and/or narrow the type so marks access
is safe.
- Around line 13-15: Initialize the test-scope variables view and container to
null instead of leaving them potentially undefined: update the declarations of
the EditorView reference (view) and the HTMLElement reference (container) so
they are explicitly set to null at declaration, and ensure any teardown logic
(e.g., in afterEach) treats them as possibly null before using or clearing them.
In `@src/components/text-editor/test-setup/mock-factories.spec.ts`:
- Around line 8-10: The variables view and container should be explicitly
initialized to null to avoid implicit undefined; update the declarations of view
(EditorView) and container (HTMLElement) in mock-factories.spec.ts to assign =
null, and adjust any afterEach/teardown guards (e.g., references that currently
check for undefined) to check for non-null before calling methods or cleaning up
to prevent runtime errors.
In `@src/components/text-editor/test-setup/mock-factories.ts`:
- Around line 56-64: The test helper currently uses "as unknown as
MockEditorView" which hides missing properties; instead declare and construct a
properly typed MockEditorView object directly: replace the casted mock with a
typed declaration (e.g. const mock: MockEditorView = { state: state ??
createEditorState(), dom: document.createElement('div'), destroy: vi.fn(),
dispatch: (tr: Transaction) => { mock.state = mock.state.apply(tr); }, ... } )
and ensure any other required fields/methods of MockEditorView are implemented
(use vi.fn() for stubs) so the object satisfies the MockEditorView shape without
double-casting; keep createEditorState, mock.dispatch, and destroy names intact.
In `@src/components/text-editor/test-setup/text-editor-test-suite-guide.md`:
- Line 54: Update the docs for the helper function createEditorView: rename the
second parameter in the signature from dispatchSpy to dispatchTransaction and
update its description to state that it accepts any dispatch callback (a
transaction handler) rather than implying a spy; ensure the signature and the
explanatory text around createEditorView(state?, dispatchTransaction?,
parentElement?) reflect this wording change so consumers understand it accepts a
dispatch callback.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 85f7f5ac-0a7f-459b-9fec-465529fa0ab2
📒 Files selected for processing (8)
src/components/text-editor/prosemirror-adapter/plugins/table-plugin.spec.tssrc/components/text-editor/prosemirror-adapter/plugins/table-plugin.tssrc/components/text-editor/test-setup/editor-test-utils.tssrc/components/text-editor/test-setup/event-simulator.spec.tssrc/components/text-editor/test-setup/event-simulator.tssrc/components/text-editor/test-setup/mock-factories.spec.tssrc/components/text-editor/test-setup/mock-factories.tssrc/components/text-editor/test-setup/text-editor-test-suite-guide.md
Comment on lines
+13
to
+15
| let view: EditorView | null; | ||
| let container: HTMLElement | null; | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Initialize test-scope refs to null.
This avoids implicit undefined state and keeps the teardown branch explicit.
🧹 Suggested tweak
- let view: EditorView | null;
- let container: HTMLElement | null;
+ let view: EditorView | null = null;
+ let container: HTMLElement | null = null;📝 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.
Suggested change
| let view: EditorView | null; | |
| let container: HTMLElement | null; | |
| let view: EditorView | null = null; | |
| let container: HTMLElement | null = null; | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/text-editor/test-setup/event-simulator.spec.ts` around lines
13 - 15, Initialize the test-scope variables view and container to null instead
of leaving them potentially undefined: update the declarations of the EditorView
reference (view) and the HTMLElement reference (container) so they are
explicitly set to null at declaration, and ensure any teardown logic (e.g., in
afterEach) treats them as possibly null before using or clearing them.
Comment on lines
+41
to
+46
| const textNode = view.state.doc.firstChild?.firstChild; | ||
| expect(textNode).toBeDefined(); | ||
|
|
||
| const hasStrongMark = textNode.marks.some( | ||
| (m) => m.type.name === 'strong' | ||
| ); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Guard textNode before reading marks for clearer failures.
If the document shape changes, Line 44 can throw a less actionable error than an explicit guard/assertion.
🛠️ Suggested test hardening
const textNode = view.state.doc.firstChild?.firstChild;
expect(textNode).toBeDefined();
+ if (!textNode) {
+ throw new Error('Expected a selected text node in first paragraph');
+ }
const hasStrongMark = textNode.marks.some(
(m) => m.type.name === 'strong'
);📝 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.
Suggested change
| const textNode = view.state.doc.firstChild?.firstChild; | |
| expect(textNode).toBeDefined(); | |
| const hasStrongMark = textNode.marks.some( | |
| (m) => m.type.name === 'strong' | |
| ); | |
| const textNode = view.state.doc.firstChild?.firstChild; | |
| expect(textNode).toBeDefined(); | |
| if (!textNode) { | |
| throw new Error('Expected a selected text node in first paragraph'); | |
| } | |
| const hasStrongMark = textNode.marks.some( | |
| (m) => m.type.name === 'strong' | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/text-editor/test-setup/event-simulator.spec.ts` around lines
41 - 46, The test reads textNode.marks which can throw if textNode is undefined;
after obtaining textNode (from view.state.doc.firstChild?.firstChild) add an
explicit guard/assertion before accessing marks (e.g., keep or strengthen
expect(textNode).toBeDefined() and only compute hasStrongMark after that
assertion) so the failure message is clear; update the code that computes
hasStrongMark (referencing textNode and hasStrongMark) to occur after the guard
and/or narrow the type so marks access is safe.
Comment on lines
+8
to
+10
| let view: EditorView | null; | ||
| let container: HTMLElement | null; | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Initialize shared test references to null explicitly.
This removes the implicit undefined state and makes teardown guards easier to reason about.
🧹 Small cleanup
- let view: EditorView | null;
- let container: HTMLElement | null;
+ let view: EditorView | null = null;
+ let container: HTMLElement | null = null;📝 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.
Suggested change
| let view: EditorView | null; | |
| let container: HTMLElement | null; | |
| let view: EditorView | null = null; | |
| let container: HTMLElement | null = null; | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/text-editor/test-setup/mock-factories.spec.ts` around lines 8
- 10, The variables view and container should be explicitly initialized to null
to avoid implicit undefined; update the declarations of view (EditorView) and
container (HTMLElement) in mock-factories.spec.ts to assign = null, and adjust
any afterEach/teardown guards (e.g., references that currently check for
undefined) to check for non-null before calling methods or cleaning up to
prevent runtime errors.
Comment on lines
+56
to
+64
| const mock = { | ||
| state: state ?? createEditorState(), | ||
| dom: document.createElement('div'), | ||
| destroy: vi.fn(), | ||
| } as unknown as MockEditorView; | ||
|
|
||
| mock.dispatch = vi.fn((tr: Transaction) => { | ||
| mock.state = mock.state.apply(tr); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid as unknown as when building MockEditorView.
Line 60 suppresses structural checks and can hide missing fields during refactors. Build a fully typed object directly.
♻️ Safer typed construction
export function createMockEditorView(state?: EditorState): MockEditorView {
- const mock = {
+ const mock: MockEditorView = {
state: state ?? createEditorState(),
dom: document.createElement('div'),
destroy: vi.fn(),
- } as unknown as MockEditorView;
-
- mock.dispatch = vi.fn((tr: Transaction) => {
+ dispatch: vi.fn(),
+ };
+
+ mock.dispatch.mockImplementation((tr: Transaction) => {
mock.state = mock.state.apply(tr);
});
return mock;
}📝 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.
Suggested change
| const mock = { | |
| state: state ?? createEditorState(), | |
| dom: document.createElement('div'), | |
| destroy: vi.fn(), | |
| } as unknown as MockEditorView; | |
| mock.dispatch = vi.fn((tr: Transaction) => { | |
| mock.state = mock.state.apply(tr); | |
| }); | |
| const mock: MockEditorView = { | |
| state: state ?? createEditorState(), | |
| dom: document.createElement('div'), | |
| destroy: vi.fn(), | |
| dispatch: vi.fn(), | |
| }; | |
| mock.dispatch.mockImplementation((tr: Transaction) => { | |
| mock.state = mock.state.apply(tr); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/text-editor/test-setup/mock-factories.ts` around lines 56 -
64, The test helper currently uses "as unknown as MockEditorView" which hides
missing properties; instead declare and construct a properly typed
MockEditorView object directly: replace the casted mock with a typed declaration
(e.g. const mock: MockEditorView = { state: state ?? createEditorState(), dom:
document.createElement('div'), destroy: vi.fn(), dispatch: (tr: Transaction) =>
{ mock.state = mock.state.apply(tr); }, ... } ) and ensure any other required
fields/methods of MockEditorView are implemented (use vi.fn() for stubs) so the
object satisfies the MockEditorView shape without double-casting; keep
createEditorState, mock.dispatch, and destroy names intact.
|
|
||
| ### 3. Editor View Utilities | ||
|
|
||
| - `createEditorView(state?, dispatchSpy?, parentElement?)` - Creates a ProseMirror editor view with optional dispatch spy |
Contributor
There was a problem hiding this comment.
Use dispatchTransaction wording in the signature docs.
Line 54 currently names the second argument dispatchSpy, but the helper accepts any dispatch callback. Renaming this in docs avoids narrowing the API conceptually.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/text-editor/test-setup/text-editor-test-suite-guide.md` at
line 54, Update the docs for the helper function createEditorView: rename the
second parameter in the signature from dispatchSpy to dispatchTransaction and
update its description to state that it accepts any dispatch callback (a
transaction handler) rather than implying a spy; ensure the signature and the
explanatory text around createEditorView(state?, dispatchTransaction?,
parentElement?) reflect this wording change so consumers understand it accepts a
dispatch callback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a layered test utility library for ProseMirror unit tests at
src/components/text-editor/test-setup/, with utilities for schema setup,editor state and view creation, content generation, command testing, and
event simulation. Every utility has its own spec file that tests real
ProseMirror behavior, not implementation details.
Also adds test coverage for the table plugin as the first real consumer
of the suite, and includes two small fixes in
menu-commands.tsthatwere discovered during review.
Test suite modules
schema-buildereditor-state-buildereditor-view-buildercontent-generatorcommand-testerevent-simulatorAll modules are re-exported from a single barrel:
editor-test-utils.ts.What each module provides
Schema builders —
createTestSchema()gives the same schema the realeditor uses (basic + lists + strikethrough + underline).
createCustomTestSchema()lets tests toggle features on/off or addcustom marks and nodes.
Editor state builders —
createEditorState()parses HTML into aProseMirror document.
createEditorStateWithSelection(),setTextSelection(), andsetNodeSelection()set up cursors andselections for command and keymap testing.
Editor view builders —
createEditorView()mounts a realProseMirror EditorView in jsdom.
createDispatchSpy()creates a spythat optionally auto-updates the view's state on dispatch.
createMockEditorView()provides a lightweight view-shaped object forunit tests that don't need real DOM rendering.
cleanupEditorView()handles teardown.
Content generators — Convenience functions that build editor states
containing specific document structures: plain text, HTML, formatted
text with marks, bullet lists, headings, blockquotes, and code blocks.
Command tester —
testCommand()applies a ProseMirror command to astate and asserts on the result (applicability, document content, document
size).
testCommandWithView()does the same with a real view for commandsthat need DOM context.
createCommandTester()creates a reusable testfunction for a single command across multiple states.
Event simulator —
simulateKeyPress()dispatches aKeyboardEventon the editor DOM, triggering ProseMirror's keymap handling end-to-end.
simulatePaste()dispatches a paste event with clipboard data.Bug fixes included
checkForActiveWrap— was only checking the direct parent node;now walks all ancestors so wrapping detection works when the cursor is
inside nested structures (e.g. a paragraph inside a list inside a
blockquote)
getAttributesreturn type — changed fromAttrs | nulltoAttrs | undefinedto match the actual return valueKnown limitations
simulatePasteonly works with plain text in jsdom — ProseMirror's HTMLclipboard parsing does not produce content in the test environment. HTML
paste testing belongs in e2e (Playwright).
Test plan
commands fire end-to-end, pasted text appears in the document
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Documentation