Skip to content

fix: artist bio rendering on artwork screen - #13851

Open
anandaroop wants to merge 6 commits into
mainfrom
anandaroop/DI-346-markdown-fix
Open

fix: artist bio rendering on artwork screen#13851
anandaroop wants to merge 6 commits into
mainfrom
anandaroop/DI-346-markdown-fix

Conversation

@anandaroop

@anandaroop anandaroop commented Jul 29, 2026

Copy link
Copy Markdown
Member

This PR resolves DI-346

Assisted-by: Claude:Sonnet-5

Description

After getting bullied by Opus on #13842 I slept on it and decided on a different approach — fix the ReadMore component's Markdown rendering. This way the Artwork screen's AboutArtist component can benefit from that fix while remaining untouched itself.

In a nutshell:

  • Make the parsing of headings more resilient to whitespace quirks in the user-supplied markdown
  • Improve list rendering by adopting bullet markers and fixing layout issues
  • (Bonus: we can undo a two-year-old workaround for the similar component on the Artist screen, by standardizing on the shared ReadMore component instead of relying on that one-off fix)

Listed works

The common case:

Platform Before After
Android
iOS

Unlisted works

Are rendered inverted:

Platform Before After
Android
iOS

Artist screen

The About tab of the artist screen has a similar expandable artist bio, and had been patched with a one-off fix a couple of years ago. I also took this opportunity to standardize that on the ReadMore component.

Platform Before After
Android
iOS

PR Checklist

  • I have tested my changes on the following platforms:
    • Android.
    • iOS.
  • I hid my changes behind a feature flag, or they don't need one.
  • I have included screenshots or videos at least on Android, or I have not changed the UI.
  • I have added tests, or my changes don't require any.
  • I added an app state migration, or my changes do not require one.
  • I have documented any follow-up work that this PR will require, or it does not require any.
  • I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

  • I would like at least one of the reviewers to run this PR on the simulator or device.
Changelog updates

Changelog updates

Cross-platform user-facing changes

  • Improve rendering of artist bios on artwork pages.

iOS user-facing changes

Android user-facing changes

Dev changes

Need help with something? Have a look at our docs, or get in touch with us.

simple-markdown's heading rule only matches `### foo` when it's followed by
a blank line. Some content (e.g. user- or admin-authored artist bios)
often puts a heading directly above its content with no blank line,
so the heading rule never fired and the literal `###` leaked into
the rendered text via the paragraph fallback rule.

Fix: Normalize the input to insert the blank line before parsing.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
@anandaroop anandaroop self-assigned this Jul 29, 2026
@anandaroop anandaroop changed the title Anandaroop/di 346 markdown fix fix: artist bio rendering on artwork screen Jul 29, 2026
@artsy artsy deleted a comment from github-actions Bot Jul 29, 2026
Comment thread src/app/utils/renderMarkdown.tsx
Comment thread src/app/utils/renderMarkdown.tsx
Comment thread src/app/Components/ReadMore.tsx Outdated
Comment thread src/app/Components/ReadMore.tsx Outdated
anandaroop and others added 4 commits July 29, 2026 21:53
ReadMore's `list` rule spread `...basicRules.paragraph` instead of
`...basicRules.list`, a copy-paste bug that gave it paragraph's match/order
instead of the real list parser.

This made the parser treat a whole `- item` block as one inline blob
(alphabetical tie-break with paragraph), so list items rendered as
wrapped plain text with literal `- ` prefixes and no hanging indent
between the bullet and wrapped continuation lines.

Fix the rule to extend the real `basicRules.list`, and render each item as
its own bullet + content row. Also switch the bullet glyph from `-` to `•`
in both ReadMore and the shared `defaultRules()` list renderer, and give
list blocks their own trailing margin, since list parsing consumes its own
trailing blank line and therefore never gets the implicit spacer that
other blocks get from a leftover `newline` node.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Biography's existing implementation was the result of a
bugfix #9840 which…

(a) had issues, since its DIY truncation code could break text inside of
html tags or entities, potentially resulting in broken markup rendering.

(b) is no longer needed, since `ReadMore` now correctly renders the
hyperlinks that were observed to be missing in the bug report DIA-458.

We now switch the query to `biographyBlurb(format: MARKDOWN)` and render
through the widely used `ReadMore` component, which truncates on the
parsed tree instead of the raw string, and has improved heading and
list rendering thanks to the previous commits.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
@anandaroop
anandaroop force-pushed the anandaroop/DI-346-markdown-fix branch from 95e9032 to c05a201 Compare July 30, 2026 02:03
@github-actions github-actions Bot deleted a comment from claude Bot Jul 30, 2026
Comment thread src/app/utils/renderMarkdown.tsx
Comment thread src/app/Components/ReadMore.tsx
Comment thread src/app/utils/renderMarkdown.tsx
@github-actions github-actions Bot deleted a comment from claude Bot Jul 30, 2026
@artsyit

artsyit commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

This PR contains the following changes:

  • Cross-platform user-facing changes (Improve rendering of artist bios on artwork pages. - anandaroop)

Generated by 🚫 dangerJS against 1487664

Comment thread src/app/utils/renderMarkdown.tsx
Comment thread src/app/utils/renderMarkdown.tsx
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes markdown rendering in ReadMore on three fronts: ATX headings that lack a trailing blank line, list items that were parsed as one inline paragraph blob (list was spreading basicRules.paragraph), and bullet glyph/spacing. Then converts Biography off hand-rolled string truncation + HTML onto the now-correct ReadMore.

The basicRules.paragraph -> basicRules.list fix is a good catch, and dropping the DIY text.slice(0, MAX_CHARS) in Biography removes a real hazard (slicing mid-entity/mid-tag). Nice that both platforms are screenshotted and the LLM assist is disclosed.

Three things worth a look before merge.


Issues Found

🟡 Important

1. ensureBlankLineAfterHeadings rewrites content inside code fencessrc/app/utils/renderMarkdown.tsx:239

Preprocessing the raw string applies everywhere renderMarkdown runs, including inside fenced blocks. The repo has a fixture that demonstrates it: src/app/utils/markdown-kitchen-sink.md:28-43 is a fence whose body is a run of H1/H2/H3 ATX headings, all at column 0. Every one gets a blank line inserted, so that code block renders double-spaced. The kitchen-sink test only asserts node types, so it stays green.

Fixing this at the rule level sidesteps it: block parsing is sequential, so once the parser reaches the opening fence the fence rule consumes the whole block and heading never sees its contents. Take simple-markdown default heading regex, relax the trailing-newline requirement from two-or-more to one-or-more, and pass it as match on the heading rule instead of touching the source string. Details inline.

2. Headings ignore the color propsrc/app/utils/renderMarkdown.tsx:191

return (
  <Text mb={1} key={state.key} variant={variant}>
    {output(node.content, state)}
  </Text>
)

ReadMore threads color through every rule it overrides — list, link, paragraph, em, strong — but not heading, and defaultRules.heading hardcodes no color. That did not matter before this PR, because a heading line fell through to the paragraph rule, which is color-aware. Now that headings actually parse, they render in the default text color.

AboutArtist.tsx:62 passes color={textColor}, which is mono0 when artwork.isUnlisted is true and the panel background is mono100 (AboutArtist.tsx:29-30). So a bio containing a heading on an unlisted artwork gets that heading in the default color against an inverted background. Worth checking against a bio that actually has one — the unlisted screenshots in the description do not appear to include a heading. Same gap applies to blockQuote, codeBlock and inlineCode, though those are far less likely in a bio.

3. The useNewTextStyles list branch has the same copy-paste bug you just fixedsrc/app/utils/renderMarkdown.tsx:140

output(node.content, state) — a simple-markdown list node is {ordered, start, loose, items}, with no content, so this passes undefined to output and ignores item entirely. Reachable from FairMoreInfo.tsx:35, SaleInfo.tsx:56 and ShowLocationHours.tsx:7. Pre-existing and strictly out of scope, but it sits four lines from a line you are already changing and it is the exact bug this PR exists to fix. Suggestion inline.

🟢 Suggestions

  • renderMarkdown.tsx:156<Flex mb={1}>{items}</Flex> has no key, so React warns when a list appears among sibling blocks. Pre-existing (the old <View> had none either), but the line is changing anyway.
  • renderMarkdown.tsx:130 — the ordered bullet renders `${i + 1} . `: stray space before the period, and it ignores node.start, which your new ReadMore rule now honors. Worth aligning the two.
  • AboutArtist.tsx:76biographyBlurb(partnerBio: false) leaves format unspecified and relies on the Metaphysics default. Biography now asks for MARKDOWN explicitly; making AboutArtist explicit too would pin down the assumption this whole PR rests on.

Areas Reviewed

Architecture — Standardizing on ReadMore and deleting the two-year-old workaround is the right call; truncating the parsed tree cannot break markup the way text.slice() could. Keep that commit.

Bugs and edge cases — I traced truncate() against the new list structure (Flex row -> bullet Text + Flex -> content Text). textDepth still lands at 1 on the content text, so the Read more affordance attaches correctly, and rows past the cutoff are dropped by the if (truncated) filter. Looks sound.

Testing — Good additions. The ordered-list test covering a custom start value is a nice touch.


Questions for Author

  1. Biography.tsx:21 appends the credit as `${text} ${credit}` onto the raw markdown now. If a bio ends in a list, does the credit get absorbed into the last list item as a lazy continuation? The old HTML string concat was equally naive, so this may be no worse — but the failure mode is more visible now that lists render as real rows.
  2. Nested lists: the new rule renders item content inside <TextComponent>, so a sublist puts a Flex (View) inside a Text. Android is finicky about that. Do artist bios ever nest, or is it not worth guarding?

@anandaroop

Copy link
Copy Markdown
Member Author

I'm going to punt on Opus's final suggestions here as too edge-casey / out of scope / preexisting.

(I'm finding these Opus 5 reviews to be a bit overzealous tbh!)

Ready for review now cc @artsy/diamond-devs

@anandaroop
anandaroop requested a review from a team July 30, 2026 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants