fix: trim text from end when character limit exceeded #7261
Conversation
|
✅ Deploy Preview for tiptap-embed ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #7254 by changing the CharacterCount extension to trim text from the end instead of the beginning when initial content exceeds the character limit. Previously, the extension deleted characters from position 0 to over, removing the start of the content. Now it traverses the document to find where the limit is reached and deletes everything after that point.
Key changes:
- Replaced simple range deletion (
deleteRange(0, over)) with node traversal logic that finds the exact position where the character limit is reached - Added character-by-character counting within text nodes to handle the limit boundary accurately
- Preserved content from the beginning of the document up to the limit
Comments suppressed due to low confidence (1)
packages/extensions/src/character-count/character-count.ts:163
- This new trimming behavior when initial content exceeds the limit lacks test coverage. The repository has Cypress tests for other extensions (e.g.,
tests/cypress/integration/extensions/bold.spec.ts), and this critical behavior change should be tested to prevent regressions.
Consider adding a Cypress test that:
- Initializes an editor with content exceeding the character limit
- Verifies that content is trimmed from the end (not the beginning)
- Tests edge cases like multi-byte characters (emojis) and custom
textCounterfunctions - Ensures the resulting content is exactly at the limit
if (initialContentSize > limit) {
const doc = newState.doc
let charCount = 0
let limitPosition = doc.content.size
doc.nodesBetween(0, doc.content.size, (node, pos) => {
if (node.isText) {
const nodeText = node.text || ''
const nodeCharCount = this.options.textCounter(nodeText)
if (charCount + nodeCharCount <= limit) {
charCount += nodeCharCount
} else {
const remainingChars = limit - charCount
let charIndex = 0
let currentCharCount = 0
for (let i = 0; i < nodeText.length; i++) {
const substring = nodeText.slice(0, i + 1)
const substringCharCount = this.options.textCounter(substring)
if (substringCharCount > remainingChars) {
break
}
charIndex = i + 1
currentCharCount = substringCharCount
}
limitPosition = pos + 1 + charIndex
return false
}
}
})
console.warn(
`[CharacterCount] Initial content exceeded limit of ${limit} characters. Content was automatically trimmed.`,
)
const tr = newState.tr.deleteRange(limitPosition, doc.content.size)
initialEvaluationDone = true
return tr
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| currentCharCount = substringCharCount | ||
| } | ||
|
|
||
| limitPosition = pos + 1 + charIndex |
There was a problem hiding this comment.
The position calculation pos + 1 + charIndex is incorrect. In ProseMirror's nodesBetween, the pos parameter is the position before the node. For text nodes, you need to account for the opening token of the parent node.
The correct calculation should be pos + charIndex (without the + 1), because:
posis the position before the text node content startscharIndexis the offset within the text node where the limit is reached
The current implementation will delete one character less than intended, leaving content that exceeds the limit.
| limitPosition = pos + 1 + charIndex | |
| limitPosition = pos + charIndex |
| let currentCharCount = 0 | ||
|
|
||
| for (let i = 0; i < nodeText.length; i++) { | ||
| const substring = nodeText.slice(0, i + 1) | ||
| const substringCharCount = this.options.textCounter(substring) | ||
|
|
||
| if (substringCharCount > remainingChars) { | ||
| break | ||
| } | ||
|
|
||
| charIndex = i + 1 | ||
| currentCharCount = substringCharCount |
There was a problem hiding this comment.
The variable currentCharCount is assigned but never used. Consider removing it to reduce confusion.
| doc.nodesBetween(0, doc.content.size, (node, pos) => { | ||
| if (node.isText) { | ||
| const nodeText = node.text || '' | ||
| const nodeCharCount = this.options.textCounter(nodeText) | ||
|
|
||
| if (charCount + nodeCharCount <= limit) { | ||
| charCount += nodeCharCount | ||
| } else { | ||
| const remainingChars = limit - charCount | ||
| let charIndex = 0 | ||
| let currentCharCount = 0 | ||
|
|
||
| for (let i = 0; i < nodeText.length; i++) { | ||
| const substring = nodeText.slice(0, i + 1) | ||
| const substringCharCount = this.options.textCounter(substring) | ||
|
|
||
| if (substringCharCount > remainingChars) { | ||
| break | ||
| } | ||
|
|
||
| charIndex = i + 1 | ||
| currentCharCount = substringCharCount | ||
| } | ||
|
|
||
| limitPosition = pos + 1 + charIndex | ||
| return false | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
The callback in nodesBetween doesn't handle non-text nodes, which means they are skipped in the traversal. If the document contains structural nodes (paragraphs, headings, etc.) between text nodes, the position calculation will be incorrect because those nodes occupy positions in the document.
Additionally, if all text nodes fit within the limit but their total still exceeds it (due to structural node sizes in nodeSize mode, though this code appears to use textSize), limitPosition will remain at doc.content.size and nothing will be deleted, despite the content exceeding the limit.
Consider either:
- Explicitly handling all node types in the traversal, or
- Using
textBetweento get the text and calculate positions more reliably, or - Adding a fallback check after the loop to verify the limit is actually being enforced
| const substring = nodeText.slice(0, i + 1) | ||
| const substringCharCount = this.options.textCounter(substring) | ||
|
|
||
| if (substringCharCount > remainingChars) { | ||
| break | ||
| } | ||
|
|
||
| charIndex = i + 1 | ||
| currentCharCount = substringCharCount |
There was a problem hiding this comment.
The nested loop with repeated textCounter calls has O(n²) time complexity. For each character position i, you're calling textCounter(nodeText.slice(0, i + 1)), which may iterate through all characters up to i again (especially for complex counters like Intl.Segmenter).
Consider optimizing by incrementally counting characters instead of recalculating from scratch each time:
let charIndex = 0
for (let i = 0; i < nodeText.length; i++) {
const char = nodeText[i]
const charLength = this.options.textCounter(char)
if (charCount + charLength > limit) {
break
}
charCount += charLength
charIndex = i + 1
}Note: This assumes the textCounter is additive. For complex counters (like grapheme clusters), you may need a different approach, but the current implementation is still problematic for long text.
| const substring = nodeText.slice(0, i + 1) | |
| const substringCharCount = this.options.textCounter(substring) | |
| if (substringCharCount > remainingChars) { | |
| break | |
| } | |
| charIndex = i + 1 | |
| currentCharCount = substringCharCount | |
| const char = nodeText[i] | |
| const charLength = this.options.textCounter(char) | |
| if (currentCharCount + charLength > remainingChars) { | |
| break | |
| } | |
| currentCharCount += charLength | |
| charIndex = i + 1 |
| } | ||
|
|
||
| charIndex = i + 1 | ||
| currentCharCount = substringCharCount |
There was a problem hiding this comment.
The value assigned to currentCharCount here is unused.
bdbch
left a comment
bdbch
left a comment
There was a problem hiding this comment.
See the copilot comments. There are a few functional and performance related problems with the PR (bad-scaling loops, positions being calculated off, etc.)
I would also like to have a test for this.
Feel free to add a vitest unit test in packages/extensions/src/character-count/__test__ that tests those cases.
2 participants
fix: #7254
Changes Overview
Implementation Approach