fix(web): restore text selection in rendered markdown on touch devices - #1890
Merged
simple-agent-manager[bot] merged 4 commits intoAug 23, 2026
Merged
Conversation
…ection
Reported on production (Android): long-pressing text in a markdown library file
selected one word and immediately deselected it, so the drag handles could never
be used and quoting a passage was impossible. Raphaël also recalled something
similar in chat message comments — same root cause, different trigger.
RenderedMarkdown passed `components={{ ... }}` as an object literal inside its
render body. react-markdown renders each node via
`createElement(components[tag], ...)`, so every override got a NEW function
identity on every render. React therefore saw a different component *type* for
every heading, paragraph and list item and unmounted/remounted the entire
document rather than reconciling it.
A native Selection is anchored to real DOM nodes. Rebuilding those nodes drops
the selection and its handles. In the file preview the trigger is immediate: the
selection handler calls setSelection, which re-renders the modal, which rebuilt
the paragraph under the user's finger. Chat hits the same trap on every poll and
stream update.
Fix: hoist the overrides to a module-scope MARKDOWN_COMPONENTS (they close over
nothing from props) and memo the component so an unrelated parent re-render does
not re-render the document at all.
This is pre-existing — PR #1889 did not introduce it, it just added a surface
that re-renders at exactly the wrong moment. #1883 previously fixed an adjacent
symptom (the React snapshot being nulled) but not the underlying DOM churn.
Regression test asserts DOM node identity across an unrelated re-render AND that
a live Selection survives it, plus a control proving memo did not make the
component stale. Verified discriminating: both assertions fail on the pre-fix
code, the control passes on both.
Web suite 3443 passed / 0 failed; Playwright audits 10/10; typecheck and lint clean.
Process fix for the markdown-remount bug. Covers the class (a config object built in a render body and handed to a library that uses its values as component types), what breaks when node identity is lost (selection, focus, IME, scroll, media position, transitions), and the DOM-identity assertion pattern that catches it — toBe, not toEqual, because a remount serializes identically. Also records the corollary to rule 62 this bug exposed: a test can use the real trigger and still miss the failure if it completes the interaction inside one render. The Playwright audit drove a real selection and passed while the feature was unusable, because it created and read the selection in the same tick and never sat through a re-render mid-gesture.
…ate, test hole
Local architecture-reviewer findings, all fixed rather than deferred.
HIGH — I had shipped `components={{ ...MARKDOWN_COMPONENTS }}` in the very
commit that added the rule forbidding it. The reviewer verified it does not
reintroduce the remount (react-markdown resolves per-tag, and a shallow spread
copies each override by reference) but it allocates a fresh container every
render for no benefit and erodes the hoist it copies. Now passes the constant.
MEDIUM — GitDiffView.tsx carried a FOURTH independent copy of this renderer with
the same inline-components bug, live in the diff viewer's full-file markdown
mode. Selecting text there still deselected. Hoisted + memoized it too. Kept it
separate from MarkdownRenderer because its typography genuinely differs;
consolidating the copies is tracked, not silently ignored.
MEDIUM — the reviewer also found MessageBubble.tsx had ALREADY solved this
independently (hoisted overrides + memo, with a comment describing the same
mechanism). So chat *message* text was never affected — correcting my earlier
claim. Three independent hand-rolled fixes before this one is the argument for
mechanical enforcement, below.
Test hole — the DOM-stability tests drove their re-render through memo's
bail-out, so memo alone satisfied them and they could not see the spread. Added
a test that renders the UNMEMOIZED implementation, forcing a real re-render, and
a new markdown-components-identity.test.tsx that mocks react-markdown to capture
the components prop across renders and assert container + per-tag identity.
Verified discriminating: reintroducing the spread fails the container assertion
while per-tag correctly still passes, exactly matching the reviewer's analysis.
Removed a tautological identity assertion I had written (spreading copies
references, so it could never fail).
Reusable enforcement — added sam/no-inline-markdown-components to
eslint-plugin-sam, wired as an error. A prose rule was demonstrably not enough:
the commit adding .claude/rules/64 violated it in the same diff. Verified the
rule flags an inline literal AND a spread, and passes a hoisted constant.
Web suite 3446 passed / 0 failed; typecheck and lint clean.
Second, independent cause of the reported Android symptom, found by the local ui-ux-specialist and proven geometrically rather than by inspection. SelectionActionBar is `fixed bottom-0`, full width, ~97px tall, and — unlike SelectionPopover, which anchors to the selection's own rect — ignored the selection's position entirely. A diagnostic at 375x667 measured a selected paragraph at top:581/bottom:650 against a bar at top:570/bottom:667: the selection was completely inside the bar. The screenshot shows the paragraph fully hidden, surviving only as the truncated quote inside the bar itself. The lower drag handle is under there too, so the user sees a word select and then cannot extend it — the same user-visible symptom as the remount bug, from a different cause. It bites hardest in chat, where alignToBottom deliberately puts the newest message (the one most likely to be quoted) in exactly that band. Fix: latch the selection's rect alongside the quote (same reason the quote is latched — the browser may collapse the Selection before the bar renders), and flip the bar to the top when a bottom-pinned bar would overlap. Height is measured in useLayoutEffect rather than estimated, because it depends on how many lines the quote wraps to, and measuring before paint avoids a visible jump. Callers that pass no geometry keep the old bottom-pinned behaviour. The existing selection test could not catch this: it always selects the FIRST paragraph, which sits far from the bottom. Added a test that selects the lowest fully-visible paragraph, asserts the fixture actually put it in the danger band (so it cannot pass for the wrong reason), and asserts no overlap. Verified discriminating — disabling the flip fails it with 'action bar overlaps the selected text'. Audits 12/12 across both viewports; web suite 3446/0; typecheck and lint clean. Unrelated pre-existing finding: tests/playwright/chat-dom-bound-audit.spec.ts is fully red on main as well (0 passed / 6 failed locally, .sam-message-entry never renders). Not caused by this branch — verified by running it on main — and tracked separately rather than fixed here.
Contributor
|
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
Fixes a live production bug reported on Android: long-pressing text in a rendered markdown file selected one word and immediately deselected it, so the selection drag-handles could never be used and quoting a passage for a comment was impossible.
RenderedMarkdownpassed its react-markdown overrides as an object literal inside the render body. react-markdown renders each node viacreateElement(components[tag], ...), so every override got a new function identity on every render — React saw a different component type for every heading, paragraph and list item and unmounted/remounted the entire document instead of reconciling it.A native text
Selectionis anchored to real DOM nodes. Rebuilding those nodes drops the selection and its handles. In the file preview the trigger is immediate: the selection handler callssetSelection, which re-renders the modal, which rebuilt the paragraph under the user's finger.Fix: hoist the overrides to a module-scope
MARKDOWN_COMPONENTS(they close over nothing from props) andmemothe component so an unrelated parent re-render does not re-render the document at all.A second, independent cause of the same user-visible symptom was found in review and is also fixed here.
SelectionActionBarisfixed bottom-0, full width, ~97px tall, and — unlikeSelectionPopover, which anchors to the selection's own rect — ignored the selection's position entirely. Measured at 375x667: a selected paragraph attop:581/bottom:650against a bar attop:570/bottom:667, i.e. the selection was completely inside the bar, along with its lower drag handle. The bar now latches the selection rect alongside the quote and flips to the top when it would overlap. Without this, the remount fix alone would not have unblocked the reported gesture.This is pre-existing and wider than the library. PR #1889 did not introduce it — it added a surface that re-renders at exactly the wrong moment. The same component renders chat messages, which re-render on every poll and stream update, and the user independently recalled "a similar but different issue" there. PR #1883 previously fixed an adjacent symptom (the React snapshot being nulled when the browser collapsed the Selection) but not the underlying DOM churn.
Validation
pnpm lint— 0 errorspnpm typecheck— cleanpnpm test— web 3446 passed / 0 failed / 0 collection errors (289 files; 3440 before, +6 added)library-file-comments-audit+file-preview-modal-auditat 375x667 and 1280x800: 12/12 passed, no visual regressionEvery guard here was verified discriminating by reverting the relevant source and re-running:
components={{...}}restoredcomponents={{ ...MARKDOWN_COMPONENTS }}spread restoredaction bar overlaps the selected textsam/no-inline-markdown-componentsUnrelated pre-existing failure, not introduced here:
tests/playwright/chat-dom-bound-audit.spec.tsis fully red locally onmainas well (0 passed / 6 failed,.sam-message-entrynever renders). Verified by checking outmainand running it before assuming. Out of scope for this PR.Staging Verification (REQUIRED for all code changes — merge-blocking)
packages/cloud-init/,packages/vm-agent/, DNS, TLS, orscripts/deploy/.Staging Verification Evidence
Not applicable — see above. Substituted evidence, per
.claude/rules/13's requirement that non-staging evidence be stated explicitly:UI Compliance Checklist (Required for UI changes)
.codex/tmp/playwright-screenshots/End-to-End Verification (Required for multi-component changes)
Data Flow Trace
→
apps/web/src/components/library/FilePreviewModal.tsx(data-comment-anchorcontainer wrappingRenderedMarkdown)selectionchange/mouseup→
apps/web/src/components/project-message-view/comments/useCommentSelection.ts:useCommentSelection()→readSelection()setSelection({ anchorId, quote, x, y })→ React re-rendersFilePreviewModalRenderedMarkdownre-renders →components={{...}}literal yields new component types → React unmounts/remounts every node → the DOM the Selection is anchored to is destroyed → browser collapses the selection and removes the handles.Post-fix:
memoshort-circuits on unchangedcontent; even on a real re-render,MARKDOWN_COMPONENTShas stable identity so React reconciles and the nodes survive.SelectionActionBar(coarse pointer) /SelectionPopover(fine) → quoted comment.Untested Gaps
The gesture itself is untested and cannot be tested here. jsdom has no selection handles; Playwright sets selections via
document.createRange()rather than by dragging, which is precisely why the original Playwright audit passed while the feature was broken on a real thumb. The regression test proves the mechanism (a re-render no longer destroys a live Selection); it does not prove the Android gesture. That confirmation is Raphaël's device test post-deploy, by explicit agreement.Post-Mortem (Required for bug fix PRs)
What broke
Selecting text in rendered markdown was impossible on Android — one word would select and instantly deselect, leaving no handles to drag. Users could not quote a passage when commenting, on library files or (less visibly) chat messages.
Root cause
MarkdownRenderer.tsxhas passedcomponents={{ ... }}inline since it was written; it predates both commenting PRs. react-markdown'screateElement(components[tag], ...)turns a fresh object literal into fresh component types, which React can only handle by remounting. The DOM churn was invisible until a feature depended on DOM identity — a native Selection — at which point it became a hard blocker.Class of bug
An unstable prop identity that silently converts reconciliation into remounting. The tell is a config object, render-prop map, or component map constructed in a render body and handed to a library that uses its values as component types. It looks like configuration; it behaves like
keychurn. Nothing renders wrong, so it is invisible until something depends on node identity — selection, focus, scroll position, media playback, IME composition, CSS transitions.Why it wasn't caught
No test asserted DOM identity, only rendered output — and output was always correct. The Playwright audit reached the feature but not the way a thumb does: it created the selection programmatically and read it back in the same tick, never sitting through a re-render mid-gesture. This is
.claude/rules/62("a test must reach the feature the way production does") in a form that rule's examples do not yet cover: the test used the real trigger but not the real interaction duration.Process fix included in this PR
Two, because the prose rule alone demonstrably failed:
.claude/rules/64-unstable-prop-identity-remounts-subtrees.md(new) — the class, the react-markdown trap, what breaks when node identity is lost (selection, focus, IME, scroll, media position, transitions), and thetoBe-not-toEqualassertion pattern that catches it.sam/no-inline-markdown-componentsinpackages/eslint-plugin-sam, wired as an error — the mechanical check. The commit that introduced rule 64 violated rule 64 in the same diff, and three independent hand-rolled fixes for this bug already existed in the codebase before this one. Documentation was not preventing recurrence; a lint rule does.Post-mortem file
This PR description, plus the rule above.
Specialist Review Evidence (Required for agent-authored PRs)
needs-human-reviewlabel added and merge deferred to humancomponents={{ ...MARKDOWN_COMPONENTS }}in the same commit that added the rule forbidding it — fixed in 59a54a6. Found a MEDIUM: a FOURTH duplicate renderer inGitDiffView.tsxstill carrying the bug — fixed. Found that the tests drove their re-render through memo's bail-out and so could not see the spread — closed with an unmemoized-render test and a new components-identity test. Its central point, that three hand-rolled fixes for this already existed and a prose rule is not enough, is why the ESLint rule was added.SelectionActionBarcovering the selection near the viewport bottom, proven with a measured diagnostic and screenshot rather than inspection — fixed in 12ba07b with a discriminating regression test. Also flagged, and I agree it is unproven from here:readNowfiring on a synthesized Androidmouseupmay surface the bar earlier than the 280ms settle window, amplifying the overlap. Not independently verifiable in this sandbox; noted for on-device observation rather than speculatively "fixed".Exceptions (If any)
Agent Preflight (Required)
Classification
External References
Consulted the official react-markdown documentation for the
componentsprop contract, which confirms each mapped value is used as the component type for its node — the mechanism by which a new object literal forces remounts. React's official reconciliation documentation confirms that a changed element type unmounts the existing subtree rather than diffing it. No external API surface changed.Codebase Impact Analysis
apps/web/src/components/MarkdownRenderer.tsx— overrides hoisted to module scope, component memoizedapps/web/src/components/GitDiffView.tsx— fourth duplicate renderer, same fix appliedapps/web/src/components/project-message-view/comments/CommentPrimitives.tsx— selection-aware action bar placementapps/web/src/components/project-message-view/comments/useCommentSelection.ts— latch the selection rectapps/web/src/components/project-message-view/comments/useProjectMessageCommentUi.tsx,apps/web/src/components/library/FilePreviewModal.tsx— pass the geometrypackages/eslint-plugin-sam/src/rules/no-inline-markdown-components.js+eslint.config.mjs— mechanical enforcementapps/web/tests/unit/components/markdown-dom-stability.test.tsx,markdown-components-identity.test.tsx,apps/web/tests/playwright/library-file-comments-audit.spec.ts— regression tests.claude/rules/64-unstable-prop-identity-remounts-subtrees.md— new process ruleConsumers of
RenderedMarkdownare unchanged and were reviewed for staleness risk: chat message rendering, tool cards, and the library file preview inapps/web/src/components/.Documentation & Specs
N/A: no public docs site content changed.This is an internal rendering-correctness fix with no user-facing configuration, API, or setup implication. Agent-facing documentation is the new rule 64.Constitution & Risk Check
memocould in principle make the component stale. Mitigated by a control test asserting acontentchange still re-renders, and by the overrides closing over nothing from props so a shared instance cannot serve wrong values.memocomparescontent, which changes on every chunk, so streaming still re-renders. Explicitly covered in the architecture review.