fix(block-editor): stop creating emoji nodes and heal the ones already stored - #37442
Open
rjvelazco wants to merge 17 commits into
Open
fix(block-editor): stop creating emoji nodes and heal the ones already stored#37442rjvelazco wants to merge 17 commits into
rjvelazco wants to merge 17 commits into
Conversation
Raised in review on PR #37434 by a reviewer, after the spec merged. The spec rejected inheriting a neighbour's link mark on the grounds that it infers one author's intent from one payload shape. That holds for "inherit from a neighbour" in general. It does not hold for the specific signature this defect produces: a BARE emoji node between two text nodes whose link marks are equal on every attribute. That shape is a fingerprint, not an editorial choice. Applying a link over an existing emoji node marks the node too (verified), so an author who links across a symbol produces a marked node, not a bare one. And after this fix no typed symbol becomes a node at all, so the sandwich cannot be newly created. The one construction that does reach it — linking two runs separately around an already-converted symbol — requires having hit this defect first, and so lives in the same legacy population the heal repairs. The heal now inherits the link mark for that signature only. ProseMirror joins the resulting identical-mark run; the transform never merges nodes itself. The reported payload therefore renders as a single <a> with no re-save and no author action, which MEETS #37340's "already-split content renders as a single link without requiring a re-save" criterion instead of amending it. Two ACs on the issue still need amending, not three. research.md R8 had rejected this narrower rule partly on a false premise — that ProseMirror already handled the case. It handles the already-marked node, whose three text nodes match; it cannot join the sandwich, whose middle node is bare by construction. R9 records the correction, the counterexample that keeps "cannot be produced deliberately" from being overstated, and why a wrong merge here is visible and reversible rather than silent. AC-015 states the rule; AC-016 asserts every case where it must not fire — differing attrs, a non-text sibling, a block boundary, a sibling without a link, and a node carrying its own marks. AC-016 is the deliverable that keeps the single permitted inference from widening. Refs #37340 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Phase 2's T003 existed to check one assumption before the heal was written. The assumption was wrong, and it had been repeated in five artifacts. The spec claimed ProseMirror joins adjacent text nodes carrying identical marks during normalization, and concluded "the heal never merges nodes itself." Measured against the real schema in jsdom: Node.fromJSON returns three separate text(link) nodes, and so does setContent followed by getJSON. Fragment.fromJSON is `new Fragment(value.map(schema.nodeFromJSON))` — it constructs directly and never reaches Fragment.fromArray, which is the only place joining happens. The assumption survived three drafts because the editor RENDERS one <a>: the DOM serializer emits a mark run as a single element however many text nodes it spans. Document and DOM disagree, and only the DOM had been looked at. It matters because the stored JSON is what VTL and the SDKs consume, and those emit one <a> per text node — the Gap B behavior #37340 documents. An unmerged heal would store three adjacent text(link) nodes and render three anchors where the defect currently produces two. It would have made the reported symptom worse. So the heal merges. Two constraints keep that from becoming a second inference: - Marks are already identical, so concatenating decides nothing. It is precisely what Fragment.fromArray does. The link sandwich remains the only place this change infers anything. - The merge is scoped to inline arrays the heal actually touched. A document-wide pass would normalize identical-mark runs unrelated to this defect and break AC-017's identity guarantee. AC-013 gains the merge as part of the heal's definition; AC-015 drops "ProseMirror then joins the run"; AC-016 gains two negative cases — differing marks never merge, and untouched arrays are returned as found. No renumbering: every change folds into an existing criterion. Evidence in research.md R10, which also records T004: jsdom's isEmojiSupported() returns false, so the cdn.jsdelivr.net fallbackImage is the default render there rather than an edge case. R3's two paste defenses both stay. Refs #37340 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes all five paths that mint an `emoji` node. The issue names one.
1. appendTransaction — converted ANY emoji character on ANY change:
1907 of the extension's 1949 catalogued characters, 219 of which
render as ordinary typography. This also explains the toolbar
picker, which already inserted a literal character that the hook
converted straight back.
2. :shortcode: input rule
3. :) emoticon rule
4. pasted :shortcode:
5. parseHTML — not a rule, but pasting emoji HTML copied from another
Block Editor field minted a node just the same.
Paths 2 and 3 keep working; only their output changes, from a node to
the literal character in the surrounding text node. Path 5 is closed by
neutralizing parseHTML AND by a transformPastedHTML that resolves the
span before ProseMirror parses. Both are needed: upstream renders two
HTML shapes, and dropping the parse rule alone leaves the fallbackImage
variant's <img> exposed for DotImage to claim as a dotCMS image pointing
at cdn.jsdelivr.net — worse than the bug being fixed. That shape is not
hypothetical; it is the default wherever isEmojiSupported() is false.
addProseMirrorPlugins returns the double-click plugin rather than an
empty array. Upstream bundles handleDoubleClickOn with the conversion,
and only the conversion is the defect; the gesture still selects a
stored node in one click.
Two fixes found while writing the tests:
- The emoticon rule swallowed the space that triggered it. TipTap runs
input rules from handleTextInput, and a rule producing steps marks the
event handled, so the typed space never reached the document. It is
now re-inserted — an author typing "hi :) there" keeps their spacing.
- enableEmoticons no longer gates on has('emoji'). `emoji` is not
selectable in Allowed Blocks (getEditorBlockOptions offers block nodes
only, #37175), so that gate was true only on fields with NO
restriction — silently removing :) from every field that restricted
anything else. Nobody configured that. Closes AC-008 for the rule;
the toolbar half follows in US4.
74 tests: the affected class sampled across reported symbols, 20
text-presentation characters and multi-codepoint cases; start, middle
and end of linked text; four paste shapes; both emoji HTML shapes; and a
guard proving a schema without the registration still cannot parse a
stored node — the reason the registration stays.
Refs #37340
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Turns stored `emoji` nodes back into text on the editor's parse path, so the character returns to every consumer at once. The node is the wrong shape for stored content: it holds a TipTap shortcode rather than a character, and the table that resolves one ships inside an npm dependency of the editor. Nothing downstream can perform that lookup. Converting it to `text` fixes, with no change outside this library: VTL dropping the character, the SDKs rendering an unknown block, Elasticsearch never indexing it, `StoryBlockUtil.getCharCount` counting zero, and `isTextContentEmpty` reporting an emoji-only Story Block field as "empty" so required-field validation rejects it. The rule is `emoji(marks) -> text(same marks, character)`. Marks are preserved, never invented — a transform that guessed could alter documents that never had this defect, silently and irreversibly. One exception, and it is the only inference in this change: a BARE node between two text nodes carrying identical mark sets that include a link inherits those marks. That shape is a fingerprint of this defect, not an editorial choice — applying a link over a node marks the node too, and after the fix no typed symbol becomes a node at all. Nine negative cases are asserted individually, which is what keeps the inference from widening: differing href, target or aria-label; a hardBreak sibling; a sibling with no link; both block boundaries; a node carrying its own marks; and an inline array the heal never touched. Requiring the FULL mark sets to match, rather than only the link marks, is narrower than AC-015 demands. It can only fire less often, and it keeps the merge coherent — a healed node whose marks differed from its neighbours' would not merge, leaving the run split anyway. The merge is ours, per research.md R10. ProseMirror does not join identical-mark runs on any load path, and unmerged runs would render as three anchors downstream where the defect produces two. Also updates one #37175 assertion, deliberately inverted: it required `enableEmoticons` to be false on a restricted field. `emoji` is not selectable in Allowed Blocks, so that gate was true only on fields with no restriction — it could only ever misfire. Full suite: 255 tests, up from a 158 baseline. Refs #37340 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…bar (#37340) Completes AC-008. The extension half landed with US1; this is the toolbar half. `emoji` is not selectable in Allowed Blocks — the option list comes from getEditorBlockOptions(), which offers block nodes only, and link/emoji/youtube were excluded by #37175 itself. So isAllowed('emoji') was true ONLY on a field with no restriction at all: restricting any block silently removed the emoji button, and nobody configured that. It is a gate that could only misfire. Removing it makes showInsertGroup dead — the insert group now always holds at least the emoji button — so the computed and its two template guards go with it rather than being left as always-true conditions. Visible consequence, and the intended one: a heavily restricted field that previously showed no insert group now shows one containing the emoji button. The Red for this story was verified retroactively rather than up front: the gate was removed before the tests existed, so Red was confirmed by reverting the two toolbar files and re-running. The set discriminates correctly — the two restricted-field cases fail against the old gate while the unrestricted and explicit-emoji cases pass, which is exactly the boundary this AC moves. The TDD order was not followed here; noting it rather than glossing it. The toolbar store is stubbed with a Proxy rather than a hand-listed object. The component reads a wide surface of mark and block signals, and enumerating them would make the spec a maintenance burden for every future button while testing none of them. Full suite: 259 tests, 19 suites. Refs #37340 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Completes AC-010. The node/action inventory still described the old gating and said nothing about the node no longer being created. Adds a section beside the AIContent one covering the two reasons the registration is load-bearing, the heal that converts stored nodes on load, the server-side blind spots it fixes with no Java change, and two traps: parseHTML is deliberately empty, and assertions belong on the JSON rather than the editor DOM — that disagreement hid a wrong design assumption through three drafts. Refs #37340 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Open
26 tasks
github-actions
Bot
added
the
Area : Frontend
Contributor
|
Claude finished @rjvelazco's task in 1m 59s —— View job Review — PR #37442 (re-review on synchronize)
Focused this pass on the emit-on-heal path, the widened link-sandwich/run logic, the New IssuesNo issues found. Things I specifically tried to break and couldn't:
Consistent with ADR-0013's frontend-only skip: no Java/DB/REST/renderer surface touched. Jest-only, as stated. |
rjvelazco
changed the title
…37340) Found in manual testing. The heal ran and the © rendered correctly, but opening the field and hitting Publish saved the ORIGINAL value — the repair only stuck if the author also typed something first. `loadContent` calls setContent with `emitUpdate: false`, deliberately: a host push or a reactive-forms write must not look like an author edit. The consequence went unnoticed because the healed document was sitting in the editor while the form control still held the unhealed string, and the editor renders both identically. Now the load path emits when — and only when — the heal actually rewrote something. `healEmojiNodes` returns the same reference for an untouched document, so a field carrying no `emoji` node stays pristine and is never marked dirty just by being opened. That guard is the whole reason this is safe to do. Deferred to a microtask: `writeValue` is one of `loadContent`'s callers, and calling onChange synchronously inside it trips NG0100. `onUpdate` and the load path now share one `emitValue`, so both emit the identical shape. They differed before, which is how this slipped through. Also closes a real coverage gap. `emoji-heal.utils.spec.ts` tests the transform as a pure function, which cannot fail for the reason this bug existed — nothing asserted that the heal reached the host at all. The two new specs assert at the call site: a broken document emits a healed value with no author edit, and a clean one emits nothing. Verified discriminating by disabling the emit — the first fails, the second still passes. Spec amended: the "reaches storage on the author's next save" line was true only with an edit, and said so misleadingly. 261 tests, 19 suites. Refs #37340 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ode-impl' into issue-37340-block-editor-emoji-node-impl
…37340) Reverses a v2 non-goal. Suppressing the emoji node left `:copyright:` working but undiscoverable — nothing tells an author the shortcode exists. The menu is its affordance, so the rule and its discovery ship together. Small because almost nothing is new. Reused verbatim from upstream: the `:` trigger, EmojiSuggestionPluginKey so the session cannot collide with the slash menu's, and the `allow` guard. Reused from this lib: SlashMenuService for the dropdown, positioning and keyboard navigation — `open()` already takes its items as an argument, so no new component. Replaced: upstream's `command`, which inserted an emoji NODE. Leaving it would have reopened the defect through the menu itself. Ours inserts the character and keeps upstream's overrideSpace handling. Written from scratch: `items`. Upstream ships no default and never has, which is what the old `items: () => []` really was — a blank filled with nothing, not a working menu switched off. Two ranking defects, both caught by strengthening tests that were initially weak (one was tautological): - `:rocket:` returned `:astronaut:` first. A tag prefix was ranked equal to a shortcode prefix, and astronaut tags "rocket". Now four tiers: exact name/shortcode, name/shortcode prefix, tag prefix, substring. - A query of `smi` labelled rows `:grinning_face_with_closed_eyes:`. Rows used `shortcodes[0]`; `smile` ships as ["grinning_face_with_closed_eyes", "smile"]. Now labelled by `name`, which is also what attrs.name carries. Shared-component changes, called out because they affect `/` rows too: - The slash menu's label span gains `truncate`. Long shortcodes are far wider than the fixed w-72 dropdown and were giving the whole list a horizontal scrollbar. Invisible for block rows, whose labels are two words. The description span was already truncated; the label had been missed. - BlockItem gains `iconKind?: 'material' | 'glyph'`, defaulting to material. Glyph rows render bare — no bordered box and no `material-symbols-outlined`, which sets a font-family and variation settings built for Material's own ligature names. `leading-none` is what actually centres the glyph. Explicit flag rather than sniffing the string for an emoji: this is shared UI, and the next reader should see intent, not a regex. Coverage: five rows capped, tier ranking, labelling, and — the one that matters — the REAL command driven through the live plugin via a recording menu-service stub, asserting a character lands and no `emoji` node appears. Simulating the insert would have tested the assertion rather than the code. Worth knowing for anyone writing more of these: @tiptap/suggestion's plugin view declares `update` as async, so its render callbacks land in a microtask. A synchronous assertion sees the plugin state updated but the menu not yet opened, which reads as a false failure. Hence flushSuggestion(). Spec amended to v2.2 with AC-024 through AC-028, so the record does not say "deliberately not done" beside code that does it. 270 tests, 19 suites. Refs #37340 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
Added: the
|
…7340) Closes T050. Convergence found AC-016 named three non-text neighbours that must stop the link rule firing — a line break, another symbol, an image — and tested only the line break. Writing the missing two turned one of them into a rule change rather than a test. Two symbols side by side inside one link — `©®` typed together — was a payload the rule left behind. Each node's inner neighbour was the OTHER symbol rather than text, so an immediate-siblings-only check never matched. The fingerprint is identical and the run length is incidental, so the rule now scans outward past adjacent bare convertible symbols and matches on the run's boundaries. Anyone who typed two legal marks together inside a link before this fix has exactly that stored, which makes it arguably more common than the line-break case that was already covered. The scan steps over bare convertible symbols ONLY. Anything else ends the run and, not being a text node, stops the rule: a hardBreak, a dotImage, a symbol carrying its own marks, a symbol whose name does not resolve. So a symbol beside an inline image heals to bare text and stays outside the link — a picture next to a symbol says nothing about whether the symbol belonged to it. That was already the behaviour; it now has a test. Four cases added: two symbols between identical links (joins), two between different links (does not), a symbol beside a dotImage (does not), and a run broken mid-way by a hardBreak (does not) — the last proving the scan cannot tunnel through anything but symbols hunting for a text node to match. Spec amended to v2.3. AC-015 now describes a run; AC-016's non-firing list is stated in terms of the run's boundaries. Documentation drift fixed in libs/new-block-editor/CLAUDE.md, two of which the docs-converge hook would otherwise have raised: - The toolbar section still described `showInsertGroup`, deleted when the emoji Allowed Blocks gate went. Recorded along with its visible consequence: a heavily restricted field now shows an insert group it previously collapsed. - The heal's exception was described as a single bare node. - Added a section on `BlockItem.iconKind` and the truncated row label, since the `:` autocomplete introduced both to shared slash-menu surface. 274 tests, 19 suites. Refs #37340 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes T051, which turned out to be a defect rather than a verification. `normalizeEditorContent` treats any non-JSON string value as HTML. dotCMS does not store Story Block fields that way, but a host embedding the editor can pass HTML — and R3 assumed transformPastedHTML covered the emoji-span problem. It does not cover this path: it only runs on paste. Measured by loading each rendered shape through the component with parseHTML neutralized. The span carrying a character degrades fine — its own text survives. The span wrapping the fallbackImage does not: the inner <img src="cdn.jsdelivr.net/…"> is left exposed and DotImage claims it, producing node list ["paragraph","text","dotImage","paragraph", "text"]. A legal symbol silently became a third-party image embed — worse than the defect being fixed, and exactly the failure R3 predicted for a parseHTML-only defense while getting its reach wrong. The span rewrite moves out of the extension into healEmojiHtml in emoji-heal.utils.ts, beside its JSON sibling, and is called from both transformPastedHTML and the component's HTML-string branch. Returns the input unchanged when no emoji span is present, so the common case allocates nothing. Also closes T054, which found the opposite kind of problem: an exported fixture nothing asserted. REPORTED_PAYLOAD_HEALED claimed "Copyright © All rights reserved" where the heal produces "Copyright ©All rights reserved" — the source is "dotCMS Copyright " plus the node plus "All rights reserved", and the heal invents no whitespace. Corrected, and AC-021 now asserts whole-document equality against it rather than checking node count, text and marks individually, which passes even when some other part of the document changed. T052 is covered at the schema level by AC-020, which already asserts a schema without the registration cannot parse a stored emoji node — the mechanism by which the legacy editor drops it. The visual confirmation is pre-existing behaviour and stays manual rather than being claimed. Worth recording how all of this surfaced: R3 reached the right conclusion about the danger and the wrong one about its reach. Three design errors in this feature now — the ProseMirror join, the tag-vs-shortcode ranking, and this — and every one was found by testing the claim rather than re-reading the reasoning. 277 tests, 19 suites. Refs #37340 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes T053 and T045. Built from the developer's own plan — the seeding script, the context video, and their six cases — plus six gaps it did not cover. 31 cases, all Manual, all Not Run Yet. Their six cases map cleanly onto TC-001 through TC-016 and are all correct, including the two that look contradictory but are not: `link + emoji + emoji` with no trailing text keeps the symbols OUT of the link, while `link + emoji + emoji + link` on the same URL pulls them in. The difference is whether the run has a text node on both sides. What was missing: - The no-emoji control (TC-019, TC-020). The highest-risk case on the page and not about emoji at all: a heal that rewrites content unrelated to this defect is worse than the defect. TC-020 in particular checks that merely OPENING an untouched field raises no unsaved-changes prompt. - Open and Publish without typing (TC-011). Their case 2 says "on save" without saying whether an edit happened, and that distinction was a real bug until b2f086c. - The `:` autocomplete (TC-006 – TC-010), new in this PR. Their list covers the inline shortcode but not the menu. - Recursion into headings, lists, blockquotes and table cells (TC-021). Every case on their list is paragraph-level. - Restricted-field behaviour (TC-022, TC-023), including the visible consequence that an insert group now appears where it used to collapse. - The accessibility pass itself (TC-027, TC-028) — the customer's actually reported symptom. Also carries the instruction that matters most for reading results: check the stored JSON, not the rendered page. The editor draws a mark run as one <a> however many text nodes span it, and that disagreement hid a wrong design assumption through three drafts. Refs #37340 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI's `format-test` goal failed on: libs/new-block-editor/src/lib/editor/components/toolbar/toolbar.component.spec.ts libs/new-block-editor/src/lib/editor/testing/editor.testing.ts Formatting only, no behaviour change. 277 tests and lint unchanged. Cause is a gap in my own workflow rather than anything in the repo: lint-staged runs `nx format:write` over STAGED files only, so edits made to a file after the commit that formatted it escape the hook entirely. Both files were formatted when first committed and edited again afterwards. `pnpm nx format:check` reproduces it locally in seconds and would have caught this before the push. Worth running alongside test and lint from now on, since the two are not equivalent — lint passed on both files. Refs #37340 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An independent session reviewed the range and found seven issues. I
verified each against the source before agreeing; six were real and one
of its examples was wrong. Decisions recorded in review-decisions.md.
1. HEAL HAD NO ERROR BOUNDARY — the worst of the seven.
`attrsEqual(a = {}, b = {})` used default parameters, which fire only on
`undefined`, so a stored mark with "attrs": null reached
Object.keys(null) and threw. Separately "marks": {} passed the `?? []`
guard and died on `.some`. Both are reachable: data-model.md records
that hand-crafted JSON arrives through the Contentlet API because no
server-side Story Block validation exists.
The throw propagated out of writeValue, setContent never ran, and the
field rendered EMPTY over intact stored JSON — the #37145 mechanism this
spec cites twice. Arriving at that through the repair would be a poor
joke.
Fixed structurally with asMarks/asAttrs coercion so malformed shapes
cannot throw, plus a try/catch in healEmojiNodes returning the input
untouched. Fails closed at its own boundary rather than making every
caller defensive — the precedent is content-match.utils.ts.
2. THE FAST-PATH GUARD WAS NARROWER THAN ITS OWN SELECTOR.
`html.includes('data-type="emoji"')` is a literal double-quote match,
but querySelectorAll runs post-parse and matches data-type='emoji' and
DATA-TYPE="Emoji" too. Those skipped the guard entirely, reopening the
T051/R11 defect two commits after fixing it. Now a tolerant regex, kept
as a fast path rather than deleted — deleting it puts a DOMParser on
every field load and every paste.
3. THE HEAL REACHED INTO UNKNOWN-NODE CONTENT, and my ordering comment
was wrong in the opposite direction from how I wrote it.
preserveUnknownBlockNodes swallows an unknown node into
attrs.originalNode, documented as inert and byte-for-byte; the mark pass
skips dotUnsupportedBlock to honour that. Healing first rewrote emoji
nodes inside a customer's custom block before the payload was stashed,
so the "original" restored on save was not the original. The heal now
runs LAST, which makes the payload structurally unreachable rather than
relying on a name list staying in sync.
4. `/ :sm` opened two suggestion sessions against one dropdown slot.
Flagged by me while drafting and then not done. The emoji session now
yields when a slash session is active.
5. The healed emit dropped charCount/wordCount/readingTime.
withDocStats bails at `chars <= 0` and nothing had synced the count,
because this path sets content with emitUpdate: false. Stats are now
synced before emitting.
6. The HTML load path healed but never emitted, so "emitted as soon as
the heal rewrites something" held on the JSON path only.
7. A bare `:` after a space opened a menu of five arbitrary emoji.
filterEmojis now returns nothing for an empty query. The reviewer's
`Note:` and `10:30` examples do NOT trigger — allowedPrefixes defaults
to [" "], so those colons follow a letter and a digit — which narrows
the repro rather than dismissing it.
REJECTED: the claim that inheriting the full mark set is wider than
AC-015. It is the opposite — requiring both boundaries' whole sets to
match fires strictly less often than comparing href alone, and
inheritance carries the set that matched. Narrower gate, wider payload,
one decision. Now covered by a two-mark fixture either way.
Fixing 4 and 7 exposed two more of my own bugs, both caught by tests:
suppressing onStart left the menu permanently shut, because update()
never touches isOpen; and my recording test double omitted isOpen, so
the throw was swallowed by Suggestion's async plugin view and looked
exactly like "the menu never opened".
Also adds typeTextSlowly. typeText dispatches every character
synchronously, batching the async plugin updates into a state real
typing cannot produce — one menu spec was asserting against that
artifact.
Coverage added per the agreed split: the two-mark sandwich, links
differing in rel and title, an own-marks boundary, an unresolvable-name
boundary, one-sided marks, and malformed stored JSON.
284 tests, 19 suites. Full local gate now run before pushing — affected
tests, affected lint, and format:check. The two dotcms-block-editor
failures (karma missing locally, a pre-existing module-boundary lint
error) reproduce with this work stashed.
Refs #37340
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
) Closes the residual gap peer re-verification found in finding 2 — the same defect shape, in the other direction. The guard `/data-type\s*=\s*['"]?emoji/i` is case-insensitive. The selector `span[data-type="emoji"]` is not: CSS matches attribute VALUES case-sensitively and only attribute NAMES case-insensitively. So `data-type="EMOJI"` passed the guard, paid for the DOMParser, matched zero spans, and returned the HTML untouched with the jsdelivr <img> still exposed for DotImage to claim. Not a regression — the old `includes` guard rejected the same input earlier and more cheaply — but the guard and the selector disagreeing about case IS finding 2, so leaving it would have been leaving the bug half-fixed. The CSS `i` flag costs nothing and makes the regex's `/i` honest. Verified non-vacuously: with the flag removed the new uppercase-value spec fails. Four specs added covering the attribute-name and attribute-value case variants plus the no-emoji passthrough. Also documents a trap the review surfaced: SlashMenuService.update() refreshes items, anchor and command but never touches isOpen, so calling it against a closed dropdown silently does nothing visible. Its docblock now says so, and says that a caller whose first event can arrive while the menu is shut must call open(). Nothing in the API shape hinted at it, and it cost this feature an afternoon. review-decisions.md records the re-verification, the coverage movement (heal branch 75.00% → 80.24%, functions 100%), and one deferral: typing the test doubles against the service interface so the compiler catches an omitted method. Three stubs diverged from the real interface during this work, each presenting as absence rather than error. 288 tests, 19 suites. Affected tests, lint and format:check all clean. Refs #37340 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
@claude review this code |
Contributor
|
Claude finished @rjvelazco's task in 2m 53s —— View job Review — PR #37442
I focused this pass on what's landed since the previous review on 2026-09-07: the New IssuesNo issues found. Specific things I checked and why they hold up:
One thing worth double-checking against real usage rather than a logic read: No Java/DB/REST surface is touched, consistent with ADR-0013's frontend-only skip for this PR. |
…37340) The `:` autocomplete entered the spec at v2.3, after /speckit-tasks had generated the task list, so it shipped with no approval gate and no Red gate on record. Convergence phase 9 raised that; T056 closes it. Running Red retroactively found two tests that passed against an extension that does nothing: - AC-024 read `extension.options.suggestion` — upstream's own options object, present whether or not we register a plugin. It asserted TipTap's defaults survived, not that our trigger works. - AC-025 relied on `rows.every(...)`, and `[].every()` is true. It passed *because* the filter returned nothing. Both strengthened: the probe now fails 9 of 9 instead of 7 of 9. AC-028 had the same shape — it asserted on `rocket`, whose name and first shortcode are the same string, so it held whichever source the code read. Added the `smile` case the criterion actually names, where the first shortcode is `grinning_face_with_closed_eyes`. Also labels all nine assertions with the criteria they discharge, and retitles the suite, which still said "(draft)". TDD order was not followed for this story — the implementation was drafted first at the developer's request. Recorded in research.md R13 rather than glossed, as T039 did for US4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rjvelazco
enabled auto-merge
September 9, 2026 00:10
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.
Fixes #37340. PR 2 of 2 — the spec merged in #37434.
Typing
©,®or™inside linked text split the link into two anchors and lost the character on published pages. The cause is four lines of editor configuration; v1 chased it downstream into five renderers, Java and three npm packages.13 files, over half of them tests. No VTL, no Java, no SDK, no version bump.
What was actually wrong
The emoji extension replaced any character Unicode classifies as an emoji with a bare
emojinode — 1907 of the 1949 it catalogues, including 219 that render as ordinary typography (✔,⚠, arrows,▪). A bare inline atom inside a marked run ends that run, so a linked phrase becametext(link) + emoji + text(link), persisted into the stored JSON.Worse, the node stores a TipTap shortcode, never a character, and the table that resolves one ships inside an npm dependency of the editor. Nothing downstream can perform that lookup.
Four stories
<a>, no re-saveThe picker,
:copyright:and:)all keep working — only their output changes.Found while doing it
A live Java defect this fixes for free.
StoryBlockUtil.isTextContentEmptycounts only"text"nodes and feeds required-field validation atESContentletAPIImpl.java:8180— so a required Story Block field whose only content is one emoji currently fails to save as "empty."getCharCountand ES text extraction have the same blind spot. Healing the node to text fixes all three with no Java change.A wrong assumption that survived three drafts. The spec claimed ProseMirror joins adjacent identical-mark text nodes on load. It does not —
Fragment.fromJSONconstructs directly and never reachesFragment.fromArray. It looked true because the editor renders one<a>regardless. Unmerged, the heal would have stored threetext(link)nodes and rendered three anchors where the defect produces two. The merge is now ours. Evidence inresearch.mdR10.The emoticon rule was eating a keystroke. TipTap marks the text-input event handled, so the space that triggered
:)never reached the document.hi :) therenow keeps its spacing.A gate that could only misfire.
emojiis not selectable in Allowed Blocks (getEditorBlockOptions()offers block nodes only, #37175), soisAllowed('emoji')was true only on unrestricted fields — restricting any block silently removed the emoji button and:). Nobody configured that.Three things worth your attention in review
emoji-heal.utils.ts— the sandwich rule is narrower than AC-015 requires. It matches on full mark-set equality, not just thelinkmarks. It can only fire less often and it keeps the merge coherent, but it is a deviation from the written criterion.tasks.mdrather than glossed.enableEmoticons: falseon restricted fields.Tests
259 passing, up from a 158 baseline. Jest only: no Java, DB, REST, renderer or build artifact is touched, and accepted ADR-0013 skips integration/Postman/Karate for
core-web/**in the merge queue.The 9 negative cases in
emoji-heal.utils.spec.tsare the real deliverable of US3 — they are what stops the one permitted inference widening.Still outstanding
Video
video.mov
🤖 Generated with Claude Code