Skip to content

fix: address the merge review of the streaming work - #1176

Merged
hryhoriiK97 merged 1 commit into
perf/stream-measure-reusefrom
fix/merge-review
Nov 12, 2026
Merged

hryhoriiK97 merged 1 commit into
perf/stream-measure-reusefrom
fix/merge-review

Conversation

@hryhoriiK97

@hryhoriiK97 hryhoriiK97 commented Nov 12, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes from five Fable reviews of #1140 (core C++, iOS, Android, JS/TS API, docs). The reviews found no blockers. This PR fixes the correctness, docs and comment findings; the deferred items are listed in the follow-ups doc.

Fixes

  • iOS: no instance variable on the swapped-in layout manager. TextViewLayoutManager is set onto layout managers UIKit already made (object_setClass), so an ivar's storage lies past the instance. The glyph colour that fade and gradient draw with was such an ivar; it is now an associated object, like the class's other state. A test checks the class has no ivars.
  • Core: indented code fences no longer hide a link reference definition. mayDefineLinkReference accepted a closing fence at any indent but opened fences only at column 0. An indented opener, or an over-indented closer inside a fence, flipped its state, so a later [ref]: url was skipped and [ref] stayed plain text until the stream ended. A closer now counts at 0–3 spaces of indent. A fence line the scan can't follow switches fence tracking off: one that's indented, inside a quote or list, or right after a line with |. New fixtures in incremental_parse_test cover these cases and a tab-indented closer.
  • Laid-out size used only while streaming (iOS and Android). Text appended without streaming now measures in full, so it has its height on the first commit. Tested on both platforms.
  • Android: two stale-height cases.
    • A detached EnrichedMarkdownText parks its renders, so it now clears its laid-out size on detach and when it parks one.
    • When two updates arrive before the view renders the first and the stream then pauses, the view's newer laid-out size replaces the earlier laid-out answer. StreamMeasurementTest has the sequence.
  • iOS: kill switch for text tiles. Setting EnrichedMarkdownDisableTextTiles to YES in Info.plist draws text the standard way, in case a future iOS changes the private views the tiles rely on. Documented in known limitations, with the off-screen snapshot limit.
  • iOS: the text host records the markdown each render showed, not the newest. This fixes cache eviction after an image loads.
  • JS: the package entry hands streams the native module, instead of the component file doing it on import. Under Metro's inline requires, a stream created before any view rendered could keep its text in JavaScript. The web entry never sets it, so the web build still doesn't pull in react-native. (Platform files would not work here: web code imports its .web files by name, and the docs site's bundler doesn't resolve platform extensions.)

Docs

  • Both breaking-changes pages have the streamingAnimation entry. It notes that streamingConfig now applies under flavor="commonmark" too, and gives a migrate step.
  • New "Migrating from react-native-streamdown" section and checklist. react-native-streamdown is described as no longer needed rather than deprecated.
  • mediaMode added to API_REFERENCE.md. An onStreamingEnd example in the guide, and a note that it doesn't fire on web.
  • Wording fixes: the 'progressive' code block header is qualified as github-only, overstated claims are softened, a repeated sentence is removed, and the stream platform reads Both, Web.

Comments and cleanup

  • Removed benchmark figures, bug stories and hard-coded "(within 200 ms)" notes from code comments, and a PR number from a test.
  • Removed the "Review cases" headers from the core tests.
  • Fixed stale comments: BlockKinds.hpp, the two HTML-list comments, MeasureConfig, and the finishedBytes contract (it can go back after a quoted fence).
  • document_store_test's concurrency check was flaky under the thread sanitizer. A lagging thread could store an older state after the newest, and the byte budget could then drop the newest. It now checks the shared tree on one last state that all three threads parse at once.
  • Removed destinationEnd's unused stop parameter and Android's scheduleRenderIfNeeded alias.
  • The JSDoc says streaming options apply while streaming, including with a stream, and notes the macOS animation fallback.

Testing

  • Docs site builds (no broken links or anchors). Jest 1854/1854, tsc clean, Android 202/202, ktlint clean, core parser and session tests pass, iOS unit tests pass (157 executed).
  • Galaxy A16:
    • chat (github and commonmark), smooth pacing, and forest with block slots all settle at the static height;
    • onStreamingEnd fires once in each;
    • the measure step stays at 12% of the JS thread.
  • iOS simulator: forest, forest with smooth pacing, and chat with renderers settle at the static height.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@hryhoriiK97
hryhoriiK97 merged commit 852dcab into perf/stream-measure-reuse Nov 12, 2026
6 checks passed
@hryhoriiK97
hryhoriiK97 deleted the fix/merge-review branch November 12, 2026 23:33
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.

1 participant