fix: prevent recursive Tiptap updates - #502
Conversation
|
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Claude finished @gianpaj's task in 4m 15s —— View job Code review — PR #502
|
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
There was a problem hiding this comment.
ℹ️ The fix is correct and the tests are real. One rough edge on the sync path, plus a note on root-cause confidence.
Reviewed changes — full diff at 3c4b838, both files, verified against the Tiptap 3.22.4 sources in node_modules and by running the suite locally.
emitUpdate: falseon the clamp path —grok-tts-editor.tsx:381-383, thecharactersLimit + 10truncation insideonUpdate, no longer re-entersonUpdateviasetContent.emitUpdate: falseon the controlled-value sync —grok-tts-editor.tsx:405, the[editor, value]effect no longer emitsonChangewhen it pushes an externalvalueinto the editor.- Two regression tests —
grok-tts-editor.test.tsx:194-235cover external-value sync (0onChange) and over-limit paste (exactly 1onChange).
What I confirmed rather than assumed:
- Tiptap 3.22.4's
setContentreally does default toemitUpdate: trueand setspreventUpdateon the root transaction.dispatchTransactiongatesemit('update')on that root meta only, soAutoConvertGrokTags's appended transactions are silenced too — the re-entrancy is genuinely closed at both sites. @tiptap/reactre-renders off thetransactionevent, notupdate, so suppressing the update does not break rendering or theAutoConvertGrokTags/UnsupportedGrokTagHighlightplugins.- Both new tests actually fail without the fix — I reverted the two lines and got
expected "vi.fn()" to be called 1 times, but got 2 timesandexpected "vi.fn()" to not be called at all, but actually been called 1 times. This is real coverage, not theatre. grok-tts-editor.test.tsx30/30 andaudio-generator.test.tsx30/30 pass on the branch.
ℹ️ Neither changed path recursed unboundedly, so SEXYVOICE-AI-BY may not be closed by this
The PR body is already appropriately hedged about this, and the change is worth making regardless — but it is worth being explicit about how much of the Sentry group this can plausibly account for. Tracing both call sites at HEAD~1, neither re-entered more than one level: the clamp re-entry terminates because serializeGrokEditorTokens(parseGrokTtsText(text)) is length-non-increasing, so the second onUpdate always finds text === fullText; and the sync effect's current === value check is a genuine fixed point because onChange is a plain useState setter (audio-generator.tsx:1095). React's "Maximum update depth exceeded" needs roughly 50 nested updates. So what this removes is a duplicate onChange per clamped edit, not an unbounded cascade.
Technical details
# Root-cause confidence for SEXYVOICE-AI-BY
## What was checked
- `apps/web/components/grok-tts-editor.tsx:374-391` — clamp re-entry depth is 1, bounded by
`grokTipTapDocToText(grokTextToTipTapDoc(s)).length <= s.length`.
- `apps/web/components/grok-tts-editor.tsx:394-410` — sync effect reaches `current === value`
after at most one `onChange` round trip.
- `onSelectionUpdate` (lines 360-373) only writes refs, so it is not a setState vector.
- `@tiptap/react` `compareOptions` never rebinds `onUpdate`, and `useEditor` uses the default
`deps = []`, so the `editor` identity is stable across parent re-renders.
## Required outcome
- No code change requested. This is a note so the Sentry group is not assumed closed on merge.
## Open questions for the human
- If `SEXYVOICE-AI-BY` keeps firing after deploy, the remaining per-transaction React
subscribers are the untouched ones: `useUiEditorState` / the `UiState` extension
(`grok-tts-editor.tsx:250`, `354`) and `SlashDropdownMenu`. Those are the next places to look.
- Is there a Sentry breadcrumb or a repro that pins the loop to `setContent` specifically,
or is the attribution purely from the stack shape?@v0 or keep the SHA fresh with Dependabot | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using Claude Opus | 𝕏
`emitUpdate: false` stopped the recursive update loop, but it also removed the emission that used to report what the editor actually applied. `plainTextToDoc(text)` is only a request: ProseMirror coerces it to the schema and AutoConvertGrokTags can rewrite it again in an appended transaction, so the parent's value and the character counter could drift from the visible document. Extract `applyEditorContent()`, which performs the silent `setContent` and returns the text read back out of the editor plus the reset selection. Both call sites use it, and the sync effect emits the applied text whenever it differs from `value`. Because that text comes from the editor, the next effect pass hits the existing early return and this settles after one extra render. Also restore clamping for externally-set values, which `emitUpdate: false` had silently dropped, and extract the `+ 10` grace allowance into `GROK_CHARACTERS_LIMIT_GRACE`. `onChange` is read through a ref so the effect's deps stay `[editor, value]`; depending on `onChange` would let an inline parent callback re-run the effect and jump the caret on renders where `value` did not change. Tests: add coverage for normalization reconciliation (U+00A0 does not survive serialization) and for external-value clamping, both verified to fail without this change. `renderEditor` now returns `rerenderWith()` so the rerender tests stop duplicating the JSX tree and dropping props. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
ℹ️ The prior review's concern is properly fixed and the new tests are real. One scope question on the sync path, plus two nits.
Reviewed changes — the delta since the last pullfrog review at 3c4b838, i.e. commit a1eb2d5, both files, verified by running the suite and by tracing the Tiptap 3.22.4 and @tiptap/react sources in node_modules.
- Reconciled the parent against what the editor actually applied — new
applyEditorContent(grok-tts-editor.tsx:245-255) performs the silentsetContentand returns the read-backgrokTipTapDocToText(editor.getJSON()), so both call sites act on what landed rather than what they asked for. - Restored the self-heal on the sync effect —
grok-tts-editor.tsx:477-479emitsonChangeRef.current(applied.text)only when the round trip was lossy, through a ref soonChangestays out of the effect's dep array. - Extended the clamp to external
value—grok-tts-editor.tsx:455-462now runs the incoming prop throughclampTextToCharactersLimitbefore applying it. - Named the grace window —
GROK_CHARACTERS_LIMIT_GRACE(grok-tts-editor.tsx:84) replaces the inline+ 10, exported so the tests assert against it rather than a copy. - Deduplicated the test harness —
editorTree/rerenderWithreplace the hand-copied JSX rerenders, andsettleEffects()makestoHaveBeenCalledTimes(1)meaningful after awaitFor.
What I confirmed rather than assumed:
- Both new tests fail without the code they cover. Deleting the
if (applied.text !== value)block failsreports the normalized text when an external value does not survive the editor round trip; removing the sync-effectclampTextToCharactersLimitwrapper failsclamps an external value that exceeds the character limit. 32/32 pass ona1eb2d5. - The new effect-to-parent emission converges. I built a scratch controlled parent that feeds
onChangeback intovalueand measured exactly oneonChangefor both the U+00A0 normalization case and the over-limit clamp case. The fixed point is structural, not incidental:currentis read from the unmutated doc, so the second pass always hits thecurrent === valueearly return. No round-trip idempotency is required. emitUpdate: falsestill holds across the new indirection.dispatchTransactionreturns early on root-levelpreventUpdateor when no transaction changed the doc (@tiptap/core/dist/index.js:5131), so neitherAutoConvertGrokTags.appendTransactionnor themoveEditorSelectionToEndselection transaction insideapplyEditorContentcan re-enteronUpdate.onChange(text)atgrok-tts-editor.tsx:440is not a stale closure.useEditorassignsmostRecentOptions.current = optionsduring render and proxiesonUpdatethroughoptions.current(@tiptap/react/dist/index.js:300-345, 479-489), so the direct call there and theonChangeRefcall in the effect see the same prop. The ref is purely a dep-array device, as its comment says.
ℹ️ Nitpicks
grok-tts-editor.test.tsx:302— the pre-existing paste test assertstoHaveBeenCalledTimes(1)straight after awaitForwithout the newsettleEffects(), which is exactly the exposure the helper's comment describes. Adding it there would make all three single-emission assertions equally strict.- The
// settles after one extra renderclaim atgrok-tts-editor.tsx:471-472holds only while the parent stores the emitted value verbatim, whichaudio-generator.tsx:1095does. All three new tests pass a barevi.fn()that never feeds the value back, so the convergence property itself is asserted nowhere. I verified it by hand; a stateful-parent test would lock it in given this PR exists to fix an update-depth crash.
@v0 or keep the SHA fresh with Dependabot | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using Claude Opus | 𝕏
The previous commit restored character-limit clamping on the content sync
effect, but `useEditor`'s initial `content` cannot clamp, and the
`current === value` early return means an over-limit initial value is never
revisited. That left two answers for the same question: mounting with
`charactersLimit={5}` and a 40-character value kept all 40 characters and
emitted nothing, while arriving at the same value through a prop change
truncated it to 15 and overwrote the parent's state.
The asymmetry is reachable. `audio-generator.tsx` shares one `text` state
between this editor and the non-Grok one, so switching from a higher-limit
voice mounts this editor with over-limit text.
Settle on applying external values as given. The limit is a guard on user
input, not a transform on text handed down from the parent — the same
semantics as `maxLength` on the non-Grok textarea, which likewise caps
typing without truncating an existing value. Clamping on the sync path
would also silently destroy input the user can still see. Over-limit text
stays visible, turns the counter red, and blocks generation through
`textIsOverLimit` in the parent.
`onUpdate` still clamps what the user types or pastes, so
GROK_CHARACTERS_LIMIT_GRACE keeps its single call site and the helper that
wrapped it is inlined again.
Tests: replace the external-clamping test with a pair asserting that mount
and prop change agree — both keep the over-limit value and emit nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The grace allowance above a field's character limit was written four times with two different values: `+ 10` in the generate transcript editors (Grok and non-Grok) and `+ 30` in the clone text field and the Gemini style prompt. How much room a user had past the limit therefore depended on the TTS model they picked, or on whether they were cloning. Add `CHARACTERS_LIMIT_GRACE` to `lib/ui-constants.ts` and use it at all four sites. Settle on 30, the more permissive of the two, so no flow loses headroom it has today; the generate transcript editors go from 10 to 30. This only governs how far past the red counter a user can keep typing — generation is still blocked at the real limit, which the API also enforces. `GROK_CHARACTERS_LIMIT_GRACE` is removed in favour of the shared constant. Tests: derive the character-limit fixtures from the constant rather than hardcoding 510, 1010, and the clamp lengths, so the value can change in one place. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… docs] The reconciliation added in a1eb2d5 emits back to the parent, and the comment claims it settles after one extra render. That property only holds because the emitted text is read back from the editor, but every test passed a bare mock that never fed the value into `value`, so nothing exercised the loop this PR exists to prevent. Add a harness that stores what the editor emits, the way audio-generator.tsx does, and assert exactly one emission plus a parent value matching the document. Restoring `emitUpdate: true` makes it fail with two emissions, so it covers the original update-depth crash rather than just the reconciliation. Also settle pending effects in the pasted-text case before asserting a single emission, matching the other single-emission assertions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
ℹ️ The no-clamp decision is sound and the constant unification is safe — I verified the limit gate end to end. One scope question on where the clamp still bites, one test that cannot fail, and two nits.
Reviewed changes — the delta since the last pullfrog review at a1eb2d5, i.e. commits 1d29e7e1, c841c3d6, and c1dd78f0, all seven files, verified by mutating the production code and re-running the suite, and by tracing the limit checks through to the server routes.
- Dropped the external-value clamp — the sync effect at
grok-tts-editor.tsx:446now callsapplyEditorContent(editor, value)directly, so a prop change and a mount give the same answer for over-limit text. This is exactly what the prior review's open thread asked for. - Unified the grace window —
CHARACTERS_LIMIT_GRACE = 30inlib/ui-constants.tsreplaces the removedGROK_CHARACTERS_LIMIT_GRACE, and now backs all four limited textareas:grok-tts-editor.tsx:411,non-grok-editor.tsx:66,voice-selector.tsx:167, andclone-text-field.tsx:53. - Added a controlled-parent convergence test —
ControlledEditorHarness(grok-tts-editor.test.tsx:128) feedsonChangeback intovalue, closing the prior review's nit that no test exercised a stateful parent. - Replaced the clamp test with two no-clamp tests — mount and prop change are both pinned against
OVER_LIMIT_TEXT, and the paste test now targets the sharedCLAMP_THRESHOLD. - Derived the
maxlengthassertions from the constant —audio-generator.test.tsx:627,638no longer hardcode510/1010.
What I confirmed rather than assumed:
- Widening the grace from 10 to 30 cannot leak over-limit text into a paid generation. Neither
textIsOverLimitnorstyleIsOverLimit(audio-generator.tsx:367-379) reads the constant — they compare against the exact limit — and all four server routes re-check independently with no grace:api/generate-voice/route.ts:370-380,385-386,400-407,api/v1/speech/route.ts:448-453, andapi/clone-voice/route.ts:365-384vialib/clone/text-limits.ts:21-31. The JSDoc's "the API also enforces" claim holds. - No stale copies of the old grace remain. A repo-wide grep for
GROK_CHARACTERS_LIMIT_GRACE,510,1010, and bare+ 10offsets against a limit turns up nothing outside unrelated code. applies an over-limit external value without clamping itis real coverage. Re-wrapping the sync effect's argument in a clamp fails it with35 / 5against the expected 40-character content.- The file is green — 34/34 on
c1dd78f.
ℹ️ The "never destroy visible input" rule holds only until the next keystroke
The new comment at grok-tts-editor.tsx:438-445 justifies applying value as given because clamping "would silently destroy input the user can still see". But onUpdate (:415-420) clamps unconditionally on the very next edit — including a deletion — so an editor that mounted with over-limit text loses everything past charactersLimit + 30 the moment the user touches it, with the caret jumping to the end. The truncation itself predates this PR; what is new is a stated policy that the other half of the component contradicts.
Technical details
# Over-limit text is preserved on entry and destroyed on the first edit
## Affected sites
- `apps/web/components/grok-tts-editor.tsx:438-449` — the sync effect deliberately applies
`value` unclamped, and `useEditor({ content: plainTextToDoc(value) })` does the same on
mount. Over-limit text is intentionally allowed to live in the document.
- `apps/web/components/grok-tts-editor.tsx:415-420` — `onUpdate` clamps whenever
`fullText.length > charactersLimit + CHARACTERS_LIMIT_GRACE`, regardless of whether the
edit grew the text. A single backspace on a 5000-character document with a 1000-character
limit replaces the document with 1030 characters.
## Reachability
- Split mode: `enforceCharactersLimit={!shouldDisableCharactersLimit}`
(`audio-generator.tsx:1093`) is `false` while a paid user has Split audios on, so arbitrarily
long text can be entered. Turning Split off flips `enforce` back to `true` without changing
`value`, so the sync effect no-ops and the long text stays — until the next keystroke.
- The comment's own scenario ("switching from a higher-limit voice") is currently dormant:
`getCharactersLimit` (`lib/ai.ts:45-53`) caps every voice at `PAID_LIMIT = 1000`. It becomes
live again once `GEMINI_STREAMING_ENABLED` (`lib/ai.ts:82`) is restored, since `gpro31` then
reports `8192 * 4` characters.
## Required outcome
- One coherent answer for over-limit text already sitting in the document: either it survives
editing the way it survives mount, or the comment stops claiming that preserving it is the
goal.
## Suggested approach (optional)
- Clamp only when the edit increased the length past the threshold, so `onUpdate` guards
against growth rather than retroactively truncating text the user is trying to shorten.
## Open questions for the human
- Is losing the tail on the first edit acceptable given generation is already blocked by
`textIsOverLimit`, or should the split-mode toggle-off path be handled explicitly?ℹ️ Nitpicks
grok-tts-editor.test.tsx:332-334— the comment claims "a second emission here would mean the reconciliation never converges", but dropping theapplied.text !== valueguard atgrok-tts-editor.tsx:461(making the emission unconditional) leaves the test passing: the outercurrent === valueearly return already absorbs the second pass. The test does catch removal of the emission entirely, which the siblingreports the normalized text…test also covers, so the comment promises more than the assertion delivers.- The PR description still describes
GROK_CHARACTERS_LIMIT_GRACEand "the bare+ 10", and its Impact section does not mention that the grace window on both generate-page editors tripled from 10 to 30. That is a user-visible input-length change arriving under arefactor:commit — harmless per the verification above, but worth naming in the description.
@v0 or keep the SHA fresh with Dependabot | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using Claude Opus | 𝕏
| // The character limit guards what the user types or pastes; it never rewrites | ||
| // text handed down from the parent. These two cases must agree, because | ||
| // audio-generator.tsx shares one `text` state across voices, so over-limit | ||
| // text can arrive either on mount or through a prop change. Both values are | ||
| // past the clamp threshold, so a regression that clamps them would show up. | ||
| it('mounts with an over-limit value without clamping it', async () => { | ||
| const onChange = vi.fn(); | ||
|
|
||
| renderEditor({ | ||
| charactersLimit: TEST_LIMIT, | ||
| onChange, | ||
| value: OVER_LIMIT_TEXT, | ||
| }); | ||
|
|
||
| const editor = await findEditor(); | ||
| await settleEffects(); | ||
|
|
||
| expect(editor).toHaveTextContent(OVER_LIMIT_TEXT); | ||
| expect(onChange).not.toHaveBeenCalled(); | ||
| expect(screen.getByTestId('generate-character-count')).toHaveTextContent( | ||
| `${OVER_LIMIT_TEXT.length} / ${TEST_LIMIT}`, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
mounts with an over-limit value without clamping it cannot fail. I injected a clamp into the sync effect and it passed; I then clamped the mount-time content: itself and it still passed, because the effect reapplies the unclamped value on the next pass. The shared comment's promise that "a regression that clamps them would show up" holds only for the sibling prop-change test.
Technical details
# Mount no-clamp test is tautological
## Affected sites
- `apps/web/tests/components/grok-tts-editor.test.tsx:347-364` — passes under every clamp
regression I could construct.
- `apps/web/components/grok-tts-editor.tsx:432-436` — on mount `current === value` is already
true, so the effect early-returns before reaching `applyEditorContent`. The clamp line is
never executed on the mount path, which is why clamping it is unobservable here.
## Required outcome
- Either the test exercises a code path where a clamp could actually apply on mount, or it is
dropped and the comment above it is scoped to the prop-change case that does have teeth.
## Open questions for the human
- Is the intent to pin `useEditor({ content: plainTextToDoc(value) })` staying unclamped? If so,
asserting on the initial document *before* effects settle (rather than after) would make the
test sensitive to that specific regression.
Summary
GrokTTSEditor.onUpdateContext
Sentry issue
SEXYVOICE-AI-BYrecorded 15 events on current releases. Representative frames run throughkeydown, ProseMirror DOM handling, transaction dispatch, and Tiptap update emission before React reports maximum update depth. This sequence is consistent with an editor update loop, but it does not prove that every event shares this cause.GrokTTSEditorused Tiptap'ssetContentin two programmatic paths:onUpdatevalueTiptap 3.22.4 emits an update from
setContentby default (it wasfalsein v2). Those replacements could therefore invokeonUpdate, call the parentonChange, and trigger another controlled render.Implementation
Breaking the loop
Both programmatic
setContentcalls pass{ emitUpdate: false }.Reconciling after a silent write
Suppressing the update also removed the emission that used to report what the editor ended up with.
plainTextToDoc(text)is only a request: ProseMirror coerces it to the schema, andAutoConvertGrokTagscan rewrite it again in an appended transaction —preventUpdateon the root transaction silences that too. So the string handed tosetContentis not guaranteed to be what the document contains, and the parent value (which is what gets sent to generation) and the character counter could drift from the visible document.A concrete case:
serializeInlineNodestrips every U+00A0, not just theGROK_EMPTY_WRAPPING_TEXTplaceholder, so"Hello world"renders as a 10-character document while the parent keeps the 11-character original.applyEditorContent()now performs the silentsetContentand returns both the reset selection and the text read back out of the editor. Both call sites use it, and the sync effect emits the applied text whenever it differs fromvalue. Because that text comes from the editor itself, the next effect pass hits the existingcurrent === valueearly return — this settles after one extra render regardless of whether normalization is idempotent, and it corrects any normalization asymmetry rather than only the U+00A0 one.onChangeis read through a ref so the effect's dependencies stay[editor, value]. Depending ononChangedirectly would let a consumer passing an inline callback re-run the effect on unrelated renders, which callssetContentand jumps the caret to the end of the document.External values are applied as given
The character limit is a guard on user input, not a transform on text handed down from the parent.
Before this PR, an external
valuelonger than the limit was clamped viaeffect → setContent → onUpdate → clamp.emitUpdate: falseremoved that. An intermediate commit restored the clamp on the sync path, butuseEditor's initialcontentcannot clamp, and thecurrent === valueearly return means an over-limit initial value is never revisited — so the same value produced different results depending on how it arrived:valuearrives40 / 5, no emission40 / 5, no emissionThat asymmetry is reachable:
audio-generator.tsxshares onetextstate between this editor and the non-Grok one, so switching from a higher-limit voice mounts this editor with over-limit text.Both paths now apply the value as given. This matches
maxLengthon the non-Grok textarea, which caps typing at the samecharactersLimit + 10without truncating an existing value, and it avoids silently destroying input the user can still see. Over-limit text stays visible, turns the counter red, and blocks generation throughtextIsOverLimitinaudio-generator.tsx.onUpdatestill clamps what the user types or pastes, using the namedGROK_CHARACTERS_LIMIT_GRACEin place of the bare+ 10.Validation
Regression tests were confirmed to fail against the pre-fix component:
onChangecallA further pair of tests pins mount and prop change to the same answer for over-limit external text, so the asymmetry above cannot come back unnoticed.
The rerender tests no longer duplicate the full JSX tree —
renderEditorreturns arerenderWith()helper that re-renders against the resolved initial props, so a test changing one prop cannot silently drop the others (the previous copy droppedenforceCharactersLimit). Assertions that check a value was emitted exactly once now settle pending effects first, sincewaitForresolves on the first matching call.Checks under Node.js 24.14.0 and pnpm 11.16.0:
pnpm fixallpnpm type-checkpnpm test— 57 files passed; 750 tests passed and 19 skippedpnpm fixallretains five pre-existing namespace-import warnings for Sentry imports.Impact
No change to editor formatting, Grok tag conversion, API contracts, credits, billing, generation, or storage.
One user-visible behaviour change: an externally-set value that the editor normalizes is now pushed back to the parent, where previously the parent kept a string that did not match the document.
Over-limit external text is no longer clamped on the sync path. That path is currently unreachable for
GrokTTSEditor(onEnhanceTextis only wired toNonGrokPromptEditor), and the behaviour now matches what mounting with the same text has always done.Not addressed here:
serializeInlineNodestripping every U+00A0 rather than only the empty-wrapper placeholder is a pre-existing lossy round trip that predates this PR. Narrowing it would change the text sent to speech generation for any input containing a non-breaking space, so it is left for a separate change; the reconciliation above keeps the editor and parent consistent in the meantime.Monitor
SEXYVOICE-AI-BYafter deployment to confirm the event group stops recurring.