Repository navigation
feat(mobile): render messages with gpt_markdown 1.3.4 plusparse - #8945
Conversation
|
@codex review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6394cd2a1e
ℹ️ 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".
| textAlign: textAlign, | ||
| maxLines: maxLines, | ||
| inlineComponents: inlineComponents, | ||
| inlinePatterns: inlinePatterns, |
There was a problem hiding this comment.
When a bubbled message uses newly supported reference-style media such as ![shot][1]\n\n[1]: https://example.com/shot.png, this plusparse path resolves it and invokes imageBuilder, but messageBubbleSegments() and extractTrailingImageGallery() still recognize only inline  syntax. The image therefore remains inside the text bubble instead of receiving attachment corners, and multiple trailing reference images do not form a gallery; resolve reference definitions before classifying attachments or extend both preprocessors to understand this syntax.
Useful? React with 👍 / 👎.
6394cd2 to
78b9c1a
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78b9c1a6d0
ℹ️ 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".
78b9c1a to
45f75bf
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45f75bf51c
ℹ️ 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".
| // stand-ins that _MarkdownEscapeMd and the link builders restore. | ||
| final markdownContent = useMemoized( | ||
| () => shieldMarkdownProseEscapes(galleryText), | ||
| () => separateMarkdownTables(escapeMarkdownMathDelimiters(galleryText)), |
There was a problem hiding this comment.
When a newly supported reference link has a normal HTTP definition such as [docs][1]\n\n[1]: https://example.com/docs, the earlier normalizeBareLinks(content) call rewrites the definition destination into [https://example.com/docs](https://example.com/docs) before this plusparse pipeline receives it. The reference consequently resolves to that synthetic Markdown string rather than the HTTP URL, so _buildLink does not open it; exclude reference-definition destinations from bare-link normalization before enabling reference-style parsing.
Useful? React with 👍 / 👎.
| final _paragraphBreak = RegExp( | ||
| r'[ \t]*(?:$|[-*+][ \t]|\d{1,9}[.)][ \t]|#{1,6}(?:[ \t]|$)|`{3}|~{3})', | ||
| multiLine: true, |
There was a problem hiding this comment.
When an unmatched backtick precedes a setext underline, this expression does not treat --- or === as the end of the paragraph. For example, title \n---\nhttps://example.com code`` pairs the title's backtick with the one in the later paragraph, causing normalizeBareLinksto skip that URL; plusparse then renders the heading and later paragraph separately, andautolink: false` leaves the URL non-clickable. Add setext/thematic-break lines to the paragraph terminators.
Useful? React with 👍 / 👎.
45f75bf to
e3765b9
Compare
brow
removed
the
parked
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.
Changes requested at c00dcc0. There is one blocker: a quadratic backtick scan lets a single long message freeze rendering. There is also one should-fix: the mention rewrite edits code. Both are inline.
Non-blocking: with the version bump, transcript_item_widget.dart (lines 70 and 145) also moves to plusparse, but it doesn't get escapeMarkdownLiterals. So \(…\) and \[…\] in agent transcripts now render as LaTeX. Apply the escape there, or note the change in the description.
Checked and fine: link safety (entity-decoded javascript: URLs still hit the openLink allowlist), $ math off by default, the exact 1.3.4 pin, and the BSD-3 val_* dependencies.
Merge criteria: the blocker fixed, with a long-paragraph timing guard, and the mention rewrite moved onto mapMarkdownProse.
| /// cannot continue past a blank line, a list item, heading, fence or a deeper | ||
| /// block quote. | ||
| bool _hasInlineCloser(String content, int start, int delimiterLength) { | ||
| final end = _paragraphEnd(content, start); |
There was a problem hiding this comment.
🤖 Blocker: quadratic scan on long paragraphs. _hasInlineCloser now calls _paragraphEnd for every backtick opener that has no closer on its own line. _paragraphEnd walks to the end of the paragraph each time, so a long paragraph with many openers costs O(n²).
Repro at this head: mapMarkdownProse on 'a \n b' repeated, with no blank lines, takes 117 ms at 8 KB, 444 ms at 16 KB, 1.8 s at 32 KB and 7.0 s at 64 KB (Dart JIT). Main stays in single-digit milliseconds. This runs during build through normalizeBareLinks and escapeMarkdownLiterals, in MessageContent, message_preview.dart and message_media_avatar.dart. One posted message therefore stalls the UI for everyone who views that channel or its previews.
Fix: compute the paragraph end once per paragraph and reuse it while openers stay below it, the way plusparse does, and stop the run scan at that bound. One 64 KB timing guard would pin it.
There was a problem hiding this comment.
🤖 Fixed in 13d277f9da. The scan now finds each paragraph's end once and reuses it while later openers at the same quote depth sit before it. A 64 KB timing guard ('a \n b' repeated) is in link_normalizer_test.dart and fails without the fix.
| // so _MentionMd's pattern matches multi-word names as one token, in | ||
| // the bound name's case. | ||
| // Skip content inside backticks to avoid altering inline code. | ||
| final mentionParts = markdownContent.split('`'); |
There was a problem hiding this comment.
🤖 Should-fix: the mention rewrite edits code, and an odd backtick turns the rest of a message into code. The rewrite skips code by splitting on single backticks, which no longer matches what the renderer treats as code:
~~~and four-backtick fences now render as code, but their contents still get rewritten.~~~\n@Bob Smith ran it\n~~~shows and copies@bob<NBSP>smith ran it. On main,~~~wasn't code, so this is new.- With an odd number of backticks, the rebuild wraps the last part in backticks, adding a closer nobody typed. In
stray ` tick then **bold** and @Alice, everything after the tick renders as inline code: no bold, no mention chip. Desktop shows a literal backtick. This predates the PR, but at render time it undoes the PR's own unclosed-backtick fix.
Fix: run the rewrite through mapMarkdownProse(markdownContent, …) like the other prose transforms. That handles both cases.
There was a problem hiding this comment.
🤖 Fixed in 13d277f9da. The mention rewrite now runs through mapMarkdownProse, so ~~~ and long fences keep the author's text and a stray backtick stays literal. A row in message_content_test.dart covers both and fails without the fix. Also applied escapeMarkdownLiterals (with the stand-in pattern) to both transcript surfaces, with a transcript widget test.
github-actions
Bot
added
the
codex-security-review-current
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
github-actions
Bot
added
codex-security-review-current
Move message Markdown off the legacy regex pipeline, which the deprecated inlineComponents API forced, onto the CommonMark parser that 1.3 added. Mentions, custom emoji and #channel tokens become InlinePatterns; links go through inlineLinkBuilder into the existing _buildLink. Drop workarounds the new parser makes unnecessary: the escape shielding in the render path and the _MarkdownEscapeMd, _MarkdownLabelEntityMd and _MessageLinkMd components, plus the zero-width prefix before a leading handling. Keep \[ \] \( \) literal, as on desktop: plusparse hard-codes them as LaTeX delimiters, so they are written as entities before rendering. Synthesized autolink labels escape backslashes instead of using an entity, which the parser leaves undecoded in a URL-shaped label. Keep non-interactive repo/PR/issue chips as their own semantics node, and clip forum card previews instead of overflowing now that blocks lay out as a column. Co-authored-by: studio <dc2fc481343672df31a689840c09299972e704dac592e0da6f18486646a7ee3e@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Tom Brow <[email protected]>
A real-message diff against desktop's parser found three differences: - A table typed directly under a caption line showed raw pipes. GFM lets a table interrupt a paragraph; plusparse doesn't, so a blank line is inserted before such a table. - `> [!NOTE]` quotes became alerts. Desktop has no alerts, so they stay quotes. - An unclosed backtick run paired with a run in a later block, flipping later code and prose, so math-delimiter escapes leaked into code as entities. A code span now ends with its paragraph. Bubble segmentation now unescapes a link destination itself before the imeta lookup, since content no longer arrives shielded (#8940 relied on that). Also pins gpt_markdown to exactly 1.3.4. Co-authored-by: studio <dc2fc481343672df31a689840c09299972e704dac592e0da6f18486646a7ee3e@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Tom Brow <[email protected]>
A block quote interrupts a paragraph, so an unmatched backtick must not pair with one inside a quote that follows it. Otherwise the quoted bare URL skipped normalization and rendered as non-clickable text. Quoted code spans that continue on the next line at the same depth are unchanged. Co-Authored-By: Claude Sonnet 5.5 <[email protected]> Signed-off-by: Tom Brow <[email protected]>
Its only caller was the render-path escape shielding this branch removed. Signed-off-by: Tom Brow <[email protected]>
plusparse reads tilde fences, HTML comments, pipe-less table rows and `1)` list markers, which the old parser did not. Joining the edited slot to any of them put the marker inside code, a hidden comment, a table cell or a list item. Bodies with a tilde fence or HTML comment, and last lines with an unescaped pipe or a bare `1)` marker, now take the own-line marker. Signed-off-by: Tom Brow <[email protected]>
normalizeBareLinks wrapped a definition's bare URL in a Markdown link, so plusparse read the wrapped text as the destination: the reference link opened nothing and its definition line, which used to show the URL, was hidden. Definition lines now pass through unchanged. Signed-off-by: Tom Brow <[email protected]>
The block-start check ran on one prose run between code spans, so a run that began with a newline indexed before its start and threw, and a run that began after inline code mid-line looked like a line start. It now reads the line start and the lines above from the full message, and a definition run counts only when it starts at a blank line or the top. Signed-off-by: Tom Brow <[email protected]>
gpt_markdown 1.3.4 matches mention, channel and emoji patterns on the source and again on parsed text, before and after escapes decode, so \@alice, \#general and \:wave: rendered as pills with a stray backslash. They are now written as private stand-ins that a final pattern draws as the literal character. It also masks pattern matches inside link destinations and hands the URL over still masked, so a destination holding @name, #channel or :emoji: opened the placeholder. Link and image URLs are unmasked before they are used. Signed-off-by: Tom Brow <[email protected]>
`Title\n--` is a heading in CommonMark; joining the slot to `--` made it a paragraph line. Signed-off-by: Tom Brow <[email protected]>
Each definition walked back over every definition above it, so a long run took quadratic time on the UI isolate. The scan now remembers whether the previous definition counted and reuses it for the next line. Signed-off-by: Tom Brow <[email protected]>
A stand-in is now a private-use mark plus a second character, and an authored mark is written as two, so authored text never draws as a sigil and never turns off sigil escapes. Signed-off-by: Tom Brow <[email protected]>
A title holding code ended the prose run early, so the next definition looked lazy and its destination was wrapped. Signed-off-by: Tom Brow <[email protected]>
Definition-shaped text after code in a title isn't a definition, but it overwrote the memo for its line, so the next definition looked lazy. Signed-off-by: Tom Brow <[email protected]>
Find each paragraph's end once and reuse it for later backtick openers, so a long paragraph of unpaired backticks no longer scans quadratically. Run the mention rewrite through mapMarkdownProse so tilde and long fences keep the author's text and a stray backtick stays literal. Agent transcripts now render escapes the same way messages do. Signed-off-by: Tom Brow <[email protected]>
The delimiter-row pattern had two adjacent blank runs, so a long run of spaces followed by a stray character retried every split between them. Each blank run now has one place to go. Signed-off-by: Tom Brow <[email protected]>
7edc607 to
cc194e4
Compare
github-actions
Bot
removed
the
codex-security-review-current
github-actions
Bot
added
the
codex-security-review-current
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.
No blockers at cc194e4.
My two findings from c00dcc0 are fixed: the long-paragraph backtick scan is linear (64 KB guard test added), and the mention rewrite now goes through mapMarkdownProse, so fenced and inline code stay as written. Transcripts get escapeMarkdownLiterals too.
Since the last head I checked (7edc607), the only code change is the rebase over #9325. It ports main's one-line switch to the authored mention spelling into the new mapMarkdownProse callback, and nothing else from that PR is lost. Mobile CI passes on this head, including #9325's casing test.
Not checked by me: device rendering. I relied on the PR's benchmark and simulator runs for that.
Co-authored-by: mboston.Engineer <932d3b6ebea6cc83e091551ae46d1c7e7f8f7d0fa8e9cef5e6facc8544af413b@buzz.block.builderlab.xyz> * origin/main: fix(mobile): require Builderlab consent for existing identities (#8823) feat(mobile): use HTTP/2 on iOS through app-wide URLSessions (#8897) fix(relay): publish a git push only when refs changed; check the protocol by refinement (#9425) fix(mobile): show shared chats under Solo (#9348) chore(release): release Buzz Desktop version 0.5.29 (#9456) feat(mobile): render messages with gpt_markdown 1.3.4 plusparse (#8945) fix(acp): bound observer retention by bytes (#7987) (#9436) Tune release codegen for faster/smaller binaries (#9434) feat(desktop): add dismissible banner pointing to the new buzz app (#9379) fix(mobile): keep publishing presence while the app is open (#9441) feat(mobile): share into Buzz from the iOS share sheet (#9433) fix(mobile): qualify every namesake mention and keep authored mention casing (#9325) fix(mobile): match browser GIF timing in the iOS reaction sheet (#9437) fix(mobile): play short-delay animation frames at browser speed (#9432) buzz-search: read profile names without the profile_name_fields function (#9397) buzz-db: build the community deletion lock key in Rust (#9396) fix(cli): pair for the active community, skip stopped desktops, accept a bare relay host (#9417) test(mobile): run push tests without a gateway dart-define (#9423) fix(mobile): never pop Home when archiving from channel settings (#9387) perf(acp): reduce observer CPU and lock overhead (#9381) Signed-off-by: Matthew Boston <[email protected]>
…count-backfill Co-authored-by: mboston.Engineer <932d3b6ebea6cc83e091551ae46d1c7e7f8f7d0fa8e9cef5e6facc8544af413b@buzz.block.builderlab.xyz> * bostonaholic--agent-reply-count: fix(mobile): require Builderlab consent for existing identities (#8823) feat(mobile): use HTTP/2 on iOS through app-wide URLSessions (#8897) fix(relay): publish a git push only when refs changed; check the protocol by refinement (#9425) fix(mobile): show shared chats under Solo (#9348) chore(release): release Buzz Desktop version 0.5.29 (#9456) feat(mobile): render messages with gpt_markdown 1.3.4 plusparse (#8945) fix(acp): bound observer retention by bytes (#7987) (#9436) Tune release codegen for faster/smaller binaries (#9434) feat(desktop): add dismissible banner pointing to the new buzz app (#9379) fix(mobile): keep publishing presence while the app is open (#9441) feat(mobile): share into Buzz from the iOS share sheet (#9433) fix(mobile): qualify every namesake mention and keep authored mention casing (#9325) fix(mobile): match browser GIF timing in the iOS reaction sheet (#9437) fix(mobile): play short-delay animation frames at browser speed (#9432) buzz-search: read profile names without the profile_name_fields function (#9397) buzz-db: build the community deletion lock key in Rust (#9396) fix(cli): pair for the active community, skip stopped desktops, accept a bare relay host (#9417) test(mobile): run push tests without a gateway dart-define (#9423) fix(mobile): never pop Home when archiving from channel settings (#9387) perf(acp): reduce observer CPU and lock overhead (#9381) Signed-off-by: Matthew Boston <[email protected]>
Message Markdown on mobile moves from gpt_markdown 1.2.1's legacy regex pipeline to the CommonMark parser that 1.3 added ("plusparse"). Before this, passing
inlineComponents(now deprecated, removed in 2.0.0) kept every message on the legacy pipeline.What changes for users
Rendering moves closer to desktop (react-markdown + remark-gfm):
_italic_and__bold__work, backslash escapes and HTML entities decode, and[a [b] c](url)labels are correct."```npm install```"in a sentence is inline code, and four-backtick fences work.~~~fences,1)lists and CommonMark list/quote continuation render.\[,\],\(and\)stay literal, as on desktop. Plusparse hard-codes them as LaTeX delimiters, so they're written as entities before rendering.__init__now renders as bold "init", and a line over---becomes a heading, same as desktop.Device benchmark (iPhone SE, profile build, 600 synthetic messages, 3 launches per build): scrolling spends 9% less time building, and frames with more than 8 ms of build drop from 63 to 22. Jumping onto unbuilt messages is 11% cheaper, and raster time is unchanged.
App size: the release app grows about 4.6 MB uncompressed (80.6 to 85.2 MB), mostly Dart code from the new math and highlighting packages.
Before and after
Synthetic messages that reproduce differences found in real channel history, rendered through
MessageContenton main and on this branch:Demo
A scroll through synthetic messages covering common formatting (headings, lists, quotes, links, highlighted code, a table, a task list, escapes), recorded on an iPhone 17 Pro simulator running this branch:
demo-captioned.mp4
How
Mentions, custom emoji and
#channeltokens areInlinePatterns. Links go throughinlineLinkBuilderinto the existing_buildLink, so chips, media links and taps are unchanged.Removed workarounds the parser makes unnecessary:
_MarkdownEscapeMd,_MarkdownLabelEntityMdand_MessageLinkMdcomponents#channelPlain-text previews and gallery helpers keep their own escape handling.
normalizeBareLinksstays because attachment and gallery detection depend on its output. Bubble segmentation unescapes a link destination itself before its imeta lookup, since content no longer arrives shielded.Kept desktop parity where plusparse differs from GFM:
> [!NOTE]quotes render as quotes (alertBuilderreturnsasBlockQuote()), since desktop has no alertsmapMarkdownProsenow ends an unclosed backtick run at its paragraph. Before, it could pair with a run in a later block and flip code and prose for the rest of the message.Synthesized autolink labels escape backslashes instead of using
\, which the parser leaves undecoded in a URL-shaped label.Repo/PR/issue chips keep their own semantics node.
Forum card previews clip at their 120 px cap instead of overflowing: blocks now lay out as a column.
Pinned to exactly 1.3.4. 1.4.0 renders HTML by default and themes through the separate
material_uipackage, which would need a theme bridge.Validation
MessageContenton main and on this branch. 29 differed in text or styling. On inline and fenced code, the branch matched desktop's parser (remark-gfm) in 23 of them and main in 7. The review found the three parity gaps above, now fixed. The rest are CommonMark behavior that desktop shares.#channelrenders as a link, not a headingLinkButtoncheck passed even with autolink on🤖 Generated with Claude Code