Skip to content

Defer table header announcement until separator confirms - #5

Merged
ronaldmannak merged 10 commits into
mainfrom
claude/markdown-streaming-review-pp0mux
Jul 6, 2026
Merged

Defer table header announcement until separator confirms#5
ronaldmannak merged 10 commits into
mainfrom
claude/markdown-streaming-review-pp0mux

Conversation

@ronaldmannak

Copy link
Copy Markdown
Contributor

Summary

This change restructures table parsing to defer announcing table blocks and headers until the separator line confirms the candidate, enabling proper inline formatting of header cells and eliminating the need to retract events when a table candidate degrades to an unknown block.

Key Changes

  • Table parsing deferral: Table blocks and headers are no longer announced when the first line is encountered. Instead, they're buffered and only emitted when the separator line confirms the table structure. This prevents the need to retract events when a candidate fails.

  • Header cell inline parsing: Header cells now receive the same inline parsing as body cells (e.g., | **bold** | renders with bold formatting), matching CommonMark/GFM behavior. The tableHeaderCandidate event now carries [[InlineRun]] (per-cell runs) instead of [InlineRun] (flat runs).

  • Renderer simplification: Removed the cachedAttributedString and blockCharacterOffsets tracking from MarkdownRenderer. The renderer now maintains only the per-block cache and computes the full document on-demand via currentAttributedString(), eliminating O(document) character-offset walks on every chunk.

  • ViewModel deduplication: Changed input id tracking from UUID to content-derived String hashes for .text and .chunks payloads, preventing redundant re-parses when the same content is fed multiple times. Stream inputs retain unique ids to allow re-invocation on task restart.

  • Image provider hardening: Added size limits (maxDownloadByteCount = 8MB, maxCachedImages = 96), request timeouts (15s), and streaming download validation to prevent unbounded memory growth from untrusted (LLM) markdown.

  • Code highlighting optimization: Deferred syntax highlighting of fenced code blocks until the block closes, avoiding O(block²) re-highlighting during streaming.

  • Test coverage: Added three new golden tests validating inline formatting in headers, cross-chunk table degradation, and split-invariance of table parsing across arbitrary chunk boundaries.

Implementation Details

  • Table degradation now reuses the block ID for the fallback unknown block since no table events were emitted during the candidate phase.
  • The renderer's apply() method now returns Bool (whether anything changed) instead of an optional AttributedString, shifting responsibility for document assembly to callers.
  • Stream input handling captures the first factory-derived input in @State to maintain a stable task id across view re-renders, while text/chunks inputs use content-derived ids directly.

https://claude.ai/code/session_015QPz9fi1oYqh4Pg8XvVhXM

…age review

Parser/assembler correctness:
- Defer table events (blockStart/tableHeaderCandidate/tableHeaderConfirmed)
  until the separator line confirms the candidate. Events drain to the
  assembler at every chunk boundary, so the old emit-then-rewind degradation
  could not retract events once the header and separator arrived in different
  chunks: the assembler received a second blockStart for the same ID, inserted
  a duplicate entry, orphaned the stale .table entry, and drifted out of sync
  with the renderer's block positions. Degradation now replays buffered lines
  into a never-announced ID, and unconfirmed candidates are excluded from
  openBlocks. Since the assembler never surfaced unconfirmed headers to the
  renderer, deferral costs no visible latency.
- Inline-parse table header cells like row cells (event payload becomes
  [[InlineRun]]), so `| **bold** |` headers no longer render literal markers.

View-layer identity (many views in a List/LazyVStack):
- Derive MarkdownStreamingInput ids from content for .text/.chunks. Ids were
  a fresh UUID per view construction, so every parent body re-evaluation
  restarted .task(id:), which re-tokenized the full document (.text) or
  re-fed all chunks into the live pipeline, duplicating content (.chunks).
- Capture the first .stream input per view identity so parent re-renders no
  longer cancel consumption and re-invoke the factory; document that the
  factory must replay the stream because lazy containers cancel and re-fire
  the task on scroll-out/scroll-back.
- Rebuild the pipeline for .chunks re-delivery and for stream restarts, with
  a generation guard so a cancelled-but-draining consume loop cannot publish
  into the new document.

Per-chunk hot path:
- Stop maintaining the renderer's spliced full-document AttributedString; it
  cost O(document) per chunk (offsetByCharacters walk + splice) and nothing in
  the streaming path consumed it. The joined document is now built on demand.
- Highlight fenced code only when the fence closes. An open fence re-rendered
  and re-highlighted the entire accumulated text on every chunk, O(block²)
  over a stream; open fences now use the code theme's base attributes.
- Skip image-dependency rebuilds for image-free documents and sort only
  newly discovered URLs when scheduling prefetches.

Untrusted input hardening:
- URLSessionMarkdownImageProvider: LRU-cap the decoded image cache (96
  entries), cap downloads at 8 MB (Content-Length precheck plus streamed
  cap), and use request/resource timeouts by default.

Tests: golden tables updated for the deferred emission contract, plus new
regressions for formatted header cells, cross-chunk degradation adjacent to
other blocks, and an every-boundary single-shot vs streamed equivalence sweep
for a table document.

https://claude.ai/code/session_015QPz9fi1oYqh4Pg8XvVhXM

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces several performance optimizations and safety enhancements to the Markdown rendering pipeline. Key changes include deferring table event emission until validation to prevent provisional events, optimizing code block rendering by syntax highlighting only on close, removing expensive full-document string splicing during streaming, and implementing download size limits and an LRU cache for remote images. Additionally, SwiftUI view identity stability is improved by using content-derived input IDs and capturing stream inputs in state. The review feedback highlights a critical performance issue in the remote image downloader where byte-by-byte iteration over URLSession.AsyncBytes causes high async overhead, recommending chunk-based streaming instead. A minor readability improvement is also suggested to use allSatisfy instead of double negation.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread Sources/PicoMarkdownView/Renderer/URLSessionMarkdownImageProvider.swift Outdated
Comment thread Sources/PicoMarkdownView/Views/MarkdownStreamingViewModel.swift Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 44c5fdff68

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread Sources/PicoMarkdownView/Views/PicoMarkdownView.swift Outdated
Comment thread Sources/PicoMarkdownView/Views/MarkdownStreamingViewModel.swift
Comment thread Sources/PicoMarkdownView/Tokenizer/BlockStateMachine.swift
- Preserve run-level inline attributes (bold/italic/code/link fonts and
  colors) in table cells: the cell styling pass overwrote .font and
  .foregroundColor across the whole cell, clobbering inline formatting in
  header AND body cells. Base attributes now only fill ranges no run set,
  with a renderer regression test asserting bold survives in a body cell.
- Replace the captured stream input with a per-identity task token so a view
  whose input switches between stream and non-stream modes restarts its
  consume task with the current input instead of replaying a stale one.
- Track only the last consumed input id (instead of an ever-growing seen
  set) so chunks A -> B -> A re-applies A; streams update the id too so a
  repeated chunks payload after a stream is not treated as redundant.
- Download images via a URLSessionDataDelegate that receives transport-sized
  Data chunks, replacing the per-byte AsyncBytes loop; the size cap is
  enforced per chunk and oversized Content-Length responses are rejected
  before the body is read.
- Use allSatisfy instead of double negation in the image-dependency fast
  path.

https://claude.ai/code/session_015QPz9fi1oYqh4Pg8XvVhXM

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4ca0d58a02

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread Sources/PicoMarkdownView/Views/PicoMarkdownView.swift Outdated
Comment thread Sources/PicoMarkdownView/Views/MarkdownStreamingViewModel.swift Outdated
Comment thread Sources/PicoMarkdownView/Views/MarkdownStreamingViewModel.swift Outdated
claude added 2 commits July 4, 2026 17:07
- Add an optional streamID to the stream initializer (and
  MarkdownStreamingInput.stream) so callers that swap stream factories
  within the same view identity — e.g. regenerating a response in place —
  can restart consumption; a changed identity re-keys the consume task,
  while equal identities still survive re-renders without a restart.
- Record lastConsumedInputID only after the parse/render publishes, so an
  input whose processing never completed is retried when the same content
  is re-delivered instead of being skipped as already consumed.
- Publish after every chunk when replaying a .chunks input (first update as
  a full replace since the pipeline is fresh, then incremental diffs),
  restoring progressive rendering and early image prefetch for long chunk
  arrays.

https://claude.ai/code/session_015QPz9fi1oYqh4Pg8XvVhXM
Two jobs on macos-26 runners with the newest installed Xcode 26 (the
package requires Swift 6.2):
- macOS: swift test with a SwiftPM build cache keyed on Package.resolved
- iOS: xcodebuild test against a dynamically selected available iPhone
  simulator, uploading the xcresult bundle on failure

The benchmark target is excluded from both jobs; it measures throughput
and belongs to local before/after runs, not a pass/fail gate on shared
runners.

https://claude.ai/code/session_015QPz9fi1oYqh4Pg8XvVhXM
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

The rich table renderer and the horizontal-rule renderer were built on
NSTextTable / NSTextTableBlock / NSParagraphStyle.textBlocks, which are
AppKit-only — so the package never compiled for iOS despite the declared
iOS 18 platform. The new iOS CI job surfaced this on its first run.

Guard the AppKit implementations with #if canImport(AppKit) and add UIKit
fallbacks:
- Tables render as styled text rows — bold header, cells joined by a thin
  U+2502 separator — with the same run-level attribute preservation as the
  AppKit cell path. RenderedTable is still populated with per-cell content
  so a future native iOS table presentation has structured data to build on.
- Horizontal rules render as a run of connecting box-drawing glyphs in the
  secondary label color.

macOS rendering is unchanged.

https://claude.ai/code/session_015QPz9fi1oYqh4Pg8XvVhXM
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

UITextView has no textContentStorage accessor (that is NSTextView API on
macOS), so the TextKit 2 view never compiled on iOS. Reach the
NSTextContentStorage through textLayoutManager.textContentManager instead.

https://claude.ai/code/session_015QPz9fi1oYqh4Pg8XvVhXM
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

NSParagraphStyle.textBlocks and NSTextTableBlock are AppKit-only, so the
table rendering test could not compile for iOS. On iOS, assert the text
fallback's observable shape (cells joined by a U+2502 separator) instead.

https://claude.ai/code/session_015QPz9fi1oYqh4Pg8XvVhXM
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@ronaldmannak

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a64f663b0e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread Sources/PicoMarkdownView/Views/MarkdownStreamingViewModel.swift Outdated
Comment thread .github/workflows/ci.yml
Comment thread Sources/PicoMarkdownView/Renderer/URLSessionMarkdownImageProvider.swift Outdated
Comment thread AGENTS.md
- Track the most recently *started* input id instead of recording it at
  publish time. Publish-time recording made an A -> B -> A flip while B was
  still parsing drop the returning A as a duplicate, letting superseded B
  win the screen. Recording intent at start is safe: replace/consume(chunks:)
  always run to completion once started, and a newer input overwrites the id
  and wins the pipeline generation check.
- Reject image response chunks before appending when they would exceed the
  download cap, so a server that omits or understates Content-Length cannot
  force even a transient over-cap allocation.
- Update AGENTS.md's table golden example to the deferred header event
  stream and the [[InlineRun]] header cell shape.

https://claude.ai/code/session_015QPz9fi1oYqh4Pg8XvVhXM

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1ae22c7d69

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread Sources/PicoMarkdownView/Renderer/URLSessionMarkdownImageProvider.swift Outdated
The delegate-based download wrapped URLSessionDataTask in a continuation
without a cancellation handler, so cancelling a prefetch (view reset,
scroll-out) only stopped the outer Swift task while the network transfer
kept running to timeout or the byte cap. withTaskCancellationHandler now
cancels the data task, which resumes the continuation exactly once via the
delegate's completion path.

https://claude.ai/code/session_015QPz9fi1oYqh4Pg8XvVhXM

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 828a83ef6c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread Sources/PicoMarkdownView/Renderer/URLSessionMarkdownImageProvider.swift Outdated
Comment thread Sources/PicoMarkdownView/Views/MarkdownStreamingViewModel.swift
Comment thread Sources/PicoMarkdownView/Renderer/MarkdownAttributeBuilder.swift
Awaiting an unstructured Task's value does not forward the caller's
cancellation into it, so a cancelled prefetch left the shared in-flight
download running to timeout or the byte cap. In-flight downloads now carry
a claim count: each awaiter takes a claim, a cancelled awaiter releases it
via withTaskCancellationHandler, and the download task (and its underlying
URLSessionDataTask, via the handler added previously) is cancelled only
when no claims remain — so one view scrolling away cannot kill a download
another view is waiting on. A per-entry id guards cleanup against racing a
newer download for the same URL.

Also document chunks: as a one-shot delivery (each array is parsed as a
complete document) and point live streaming at stream:, in both the
initializer docs and the README.

https://claude.ai/code/session_015QPz9fi1oYqh4Pg8XvVhXM
@ronaldmannak
ronaldmannak merged commit 9532247 into main Jul 6, 2026
2 checks passed
@ronaldmannak
ronaldmannak deleted the claude/markdown-streaming-review-pp0mux branch July 6, 2026 16:41
ronaldmannak pushed a commit that referenced this pull request Jul 6, 2026
Both branches fixed the O(n²) streaming re-highlight in
MarkdownAttributeBuilder's fencedCode path: main by highlighting only
when the fence closes, this branch by highlighting live with a 16 KB
size guard (CodeHighlightingPolicy) that defers oversized blocks to a
single pass on close. Resolved by keeping the live+guard behavior —
small chat-sized blocks stay colored while streaming with bounded
per-chunk cost (and an LRU cache), and oversized blocks get main's
render-plain-then-highlight-on-close treatment via the same
blockEnded → refreshBlock path. Adopted main's structure of resolving
theme fonts/colors before the highlight branch.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NRUDZwowe1T836zcYLwQoo
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