From 369e1ef3229d5f8b49cae9ba111f72ff5dc55baf Mon Sep 17 00:00:00 2001 From: Tom Brow Date: Tue, 3 Nov 2026 08:00:33 -0800 Subject: [PATCH] perf(mobile): stop re-parsing message markdown on every rebuild (#8928) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Context Each message in a channel shows its text as Markdown. The `GptMarkdown` widget parses that text. Parsing is slow for long messages. `GptMarkdownTheme` gives Markdown colours and styles to the widgets below it. When it gets a new theme data object, it tells all `GptMarkdown` widgets below it to parse their text again. It compares the old and new objects by identity, because `GptMarkdownThemeData` has no value equality. Thus a new object with the same values also causes a parse. ## Problem Two widgets made a new theme data object on each build: - `MessageContent` made one for each message bubble. - `AppMarkdownTheme` made one for the full app. Thus each rebuild parsed all messages on the screen again. Many events cause a rebuild: a batch of profiles, a read marker, a keyboard frame. In a large channel such as #4things (1,235 members), these events occur almost all the time. ## Change - `MessageContent` keeps its bubble theme until one of its inputs changes. The inputs are: the app Markdown theme, the link colour, own or other message, the ink colour and `onSurface`. - `AppMarkdownTheme` makes new theme data only when `ThemeData` changes or when an outer Markdown theme changes. A change of colour, of brightness, or of own or other message still makes new theme data. Then the Markdown parses again with the new colours. This is correct. ### Related issue None found. ## Testing - New regression tests fail on main. They pass on this branch. - `message_content_stability_test.dart`: When a bubble rebuilds with the same inputs, it keeps the same theme object and the same parsed `TextSpan`. The test does this for own messages and for other messages. When own or other changes, the bubble gets a new theme. - `app_markdown_theme_test.dart`: The app keeps one theme object when it rebuilds. It makes a new one when light or dark mode changes. - `just mobile-install mobile-check mobile-test` passes. - Device test: iPhone 12 mini, Profile build, Tom's Block community, open #4things. The two runs had different live traffic and different cached content. Thus they show the cause and the fix. They do not give a controlled speed measurement. - Before: In the first 2.4 s, parses from rebuilds (`performRebuild`) used approximately 90 ms. One frame used 116 ms. Approximately 48 ms of that frame parsed visible messages again. - After: In 30 s, rebuilds caused no parses. Only the first build of each message (`_firstBuild`) parsed. - An independent agent review found no defects. It found one small item: `ChatColors` has no value equality. Thus a rebuild of `MaterialApp` still makes new app theme data one time. This costs only a little performance. We did not change it. Not tested: A human test did not occur. The mobile testing exception waives it, and an agent did the device test. No test occurred on an Android device. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Tom Brow Co-authored-by: Tom Brow Co-authored-by: studio Co-authored-by: Claude Opus 5.5 --- .../features/channels/message_content.dart | 74 +++++++------------ .../lib/shared/theme/app_markdown_theme.dart | 22 ++++-- .../message_content_stability_test.dart | 54 ++++++++++++++ .../shared/theme/app_markdown_theme_test.dart | 44 +++++++++++ 4 files changed, 140 insertions(+), 54 deletions(-) diff --git a/mobile/lib/features/channels/message_content.dart b/mobile/lib/features/channels/message_content.dart index 096b1b8a01..6a6bd270cc 100644 --- a/mobile/lib/features/channels/message_content.dart +++ b/mobile/lib/features/channels/message_content.dart @@ -209,6 +209,32 @@ class MessageContent extends HookConsumerWidget { // Own-bubble links share the bubble ink; markdown marks them by underline. final linkColor = ownMessage ? ownTextColor : context.colors.tertiary; final bubbleInk = ownMessage ? ownTextColor : context.colors.onSurface; + final onSurface = context.colors.onSurface; + // GptMarkdownTheme notifies its dependents whenever the data object + // changes (the data has no value equality), and each notification makes + // every GptMarkdown below re-parse its text. Reuse one bubble theme while + // its inputs are unchanged so an ordinary rebuild does not re-parse. + final bubbleMarkdownTheme = useMemoized( + () => markdownTheme.copyWith( + linkColor: linkColor, + // A neutral chip would put white ink on a light fill; tint the accent + // instead. + inlineCode: ownMessage + ? markdownTheme.inlineCode.copyWith( + color: ownTextColor, + backgroundColor: chatCodeTint(ownTextColor, 0.18), + borderColor: ownTextColor.withValues(alpha: 0.3), + ) + : null, + h1: markdownTheme.h1?.copyWith(color: bubbleInk), + h2: markdownTheme.h2?.copyWith(color: bubbleInk), + h3: markdownTheme.h3?.copyWith(color: bubbleInk), + h4: markdownTheme.h4?.copyWith(color: bubbleInk), + h5: markdownTheme.h5?.copyWith(color: bubbleInk), + h6: markdownTheme.h6?.copyWith(color: bubbleInk), + ), + [markdownTheme, linkColor, ownMessage, ownTextColor, onSurface], + ); final baseTextStyle = (baseStyle ?? context.textTheme.bodyMedium?.copyWith( @@ -474,53 +500,7 @@ class MessageContent extends HookConsumerWidget { ), ), child: GptMarkdownTheme( - gptThemeData: markdownTheme.copyWith( - linkColor: linkColor, - // A neutral chip would put white ink on a light - // fill; tint the accent instead. - inlineCode: ownMessage - ? markdownTheme.inlineCode.copyWith( - color: ownTextColor, - backgroundColor: chatCodeTint( - ownTextColor, - 0.18, - ), - borderColor: ownTextColor.withValues( - alpha: 0.3, - ), - ) - : null, - h1: markdownTheme.h1?.copyWith( - color: ownMessage - ? ownTextColor - : context.colors.onSurface, - ), - h2: markdownTheme.h2?.copyWith( - color: ownMessage - ? ownTextColor - : context.colors.onSurface, - ), - h3: markdownTheme.h3?.copyWith( - color: ownMessage - ? ownTextColor - : context.colors.onSurface, - ), - h4: markdownTheme.h4?.copyWith( - color: ownMessage - ? ownTextColor - : context.colors.onSurface, - ), - h5: markdownTheme.h5?.copyWith( - color: ownMessage - ? ownTextColor - : context.colors.onSurface, - ), - h6: markdownTheme.h6?.copyWith( - color: ownMessage - ? ownTextColor - : context.colors.onSurface, - ), - ), + gptThemeData: bubbleMarkdownTheme, child: renderMarkdown(segment.text), ), ), diff --git a/mobile/lib/shared/theme/app_markdown_theme.dart b/mobile/lib/shared/theme/app_markdown_theme.dart index 27e5060901..ec5767db19 100644 --- a/mobile/lib/shared/theme/app_markdown_theme.dart +++ b/mobile/lib/shared/theme/app_markdown_theme.dart @@ -1,4 +1,5 @@ import 'package:flutter/material.dart'; +import 'package:flutter_hooks/flutter_hooks.dart'; import 'package:gpt_markdown/gpt_markdown.dart'; import 'app_theme.dart'; @@ -15,16 +16,23 @@ import 'code_style.dart'; /// ambient headings carry the localized text geometry, which exists only once /// `Theme.of` has run and a `ThemeData` factory cannot see. Overriding the one /// field on the ambient theme is what leaves non-code Markdown untouched. -class AppMarkdownTheme extends StatelessWidget { +class AppMarkdownTheme extends HookWidget { const AppMarkdownTheme({super.key, required this.child}); final Widget child; @override Widget build(BuildContext context) { - final scheme = Theme.of(context).colorScheme; - return GptMarkdownTheme( - gptThemeData: GptMarkdownTheme.of(context).copyWith( + final theme = Theme.of(context); + final outer = context + .dependOnInheritedWidgetOfExactType() + ?.gptThemeData; + // GptMarkdownTheme notifies by data identity, and every notification makes + // each GptMarkdown in the app re-parse its text. Derive new data only when + // what it is derived from changes. + final data = useMemoized(() { + final scheme = theme.colorScheme; + return GptMarkdownTheme.of(context).copyWith( // Inline code is otherwise drawn in gpt_markdown's own bundled face, at // its own size, tinted from `onSurface`. Stating the app's code style // gives every Markdown surface the face, size and chip colours a fenced @@ -39,8 +47,8 @@ class AppMarkdownTheme extends StatelessWidget { // scale rather than the `Radii.card` a block uses. borderRadius: const Radius.circular(Radii.xs), ).resolve(scheme), - ), - child: child, - ); + ); + }, [theme, outer]); + return GptMarkdownTheme(gptThemeData: data, child: child); } } diff --git a/mobile/test/features/channels/message_content_stability_test.dart b/mobile/test/features/channels/message_content_stability_test.dart index 08762340fd..598951dfc5 100644 --- a/mobile/test/features/channels/message_content_stability_test.dart +++ b/mobile/test/features/channels/message_content_stability_test.dart @@ -22,6 +22,8 @@ class _Fixture { bool scaleEmoji = false; bool mentionHandler = true; bool mediaHandlers = false; + bool bubble = false; + bool ownMessage = false; int? tappedRevision; String? tappedId; @@ -40,6 +42,8 @@ class _Fixture { agentMentionPubkeys: Set.of(agents), tags: tags, scaleEmojiOnly: scaleEmoji, + bubble: bubble, + ownMessage: ownMessage, onMediaReply: mediaHandlers ? () { tappedRevision = generation; @@ -79,7 +83,57 @@ List _components(WidgetTester tester) => tester .widget(find.byType(GptMarkdown).first) .inlineComponents!; +GptMarkdownThemeData _bubbleTheme(WidgetTester tester) => tester + .widget( + find + .ancestor( + of: find.byType(GptMarkdown).first, + matching: find.byType(GptMarkdownTheme), + ) + .first, + ) + .gptThemeData; + +InlineSpan _renderedText(WidgetTester tester) => tester + .widget( + find + .descendant( + of: find.byType(GptMarkdown).first, + matching: find.byType(RichText), + ) + .first, + ) + .text; + void main() { + for (final own in [false, true]) { + testWidgets( + 'a bubble rebuild with unchanged inputs keeps its parsed markdown ' + '(own: $own)', + (tester) async { + final fixture = _Fixture() + ..bubble = true + ..ownMessage = own + ..content = '# Title\n\nHello `code` and #sample'; + addTearDown(fixture.revision.dispose); + await tester.pumpWidget(fixture.build()); + final theme = _bubbleTheme(tester); + final text = _renderedText(tester); + for (var i = 0; i < 3; i++) { + await fixture.refresh(tester); + // A new theme object would notify every GptMarkdown below it, and + // each one would parse its text again. + expect(_bubbleTheme(tester), same(theme)); + expect(_renderedText(tester), same(text)); + } + fixture.ownMessage = !own; + await fixture.refresh(tester); + expect(_bubbleTheme(tester), isNot(same(theme))); + expect(_bubbleTheme(tester).linkColor, isNot(theme.linkColor)); + }, + ); + } + testWidgets('explicit channel links retain the current callback', ( tester, ) async { diff --git a/mobile/test/shared/theme/app_markdown_theme_test.dart b/mobile/test/shared/theme/app_markdown_theme_test.dart index 26ae369821..72c163ba49 100644 --- a/mobile/test/shared/theme/app_markdown_theme_test.dart +++ b/mobile/test/shared/theme/app_markdown_theme_test.dart @@ -33,6 +33,50 @@ Future _markdownTheme( } void main() { + testWidgets('keeps one theme object until the app theme changes', ( + tester, + ) async { + final theme = ValueNotifier(AppTheme.light()); + final revision = ValueNotifier(0); + addTearDown(theme.dispose); + addTearDown(revision.dispose); + final reads = []; + await tester.pumpWidget( + ValueListenableBuilder( + valueListenable: theme, + builder: (_, data, _) => MaterialApp( + theme: data, + home: ValueListenableBuilder( + valueListenable: revision, + // A fresh subtree each time, as an ordinary ancestor rebuild gives. + builder: (_, generation, _) => AppMarkdownTheme( + child: Builder( + key: ValueKey(generation), + builder: (context) { + reads.add(GptMarkdownTheme.of(context)); + return const SizedBox.shrink(); + }, + ), + ), + ), + ), + ), + ); + await tester.pumpAndSettle(); + final first = reads.last; + for (var i = 0; i < 3; i++) { + revision.value++; + await tester.pump(); + // GptMarkdownTheme notifies by identity: a new object would make every + // GptMarkdown in the app parse its text again. + expect(reads.last, same(first)); + } + theme.value = AppTheme.dark(); + await tester.pumpAndSettle(); + expect(reads.last, isNot(same(first))); + expect(reads.last.inlineCode.color, AppTheme.dark().colorScheme.onSurface); + }); + testWidgets('states inline code and leaves the rest of Markdown alone', ( tester, ) async {