Repository navigation
Conversation
Codex Review SummaryThis 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. |
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a small, localized mobile performance optimization that preserves existing Markdown/HTML conversion behavior and adds focused regression coverage. It introduces no product-default, schema, deployment, security, billing, or static-analysis changes. You can add or adjust custom eligibility rules. Learn more. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the
Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Code-range-aware conversion apps/mobile/src/lib/markdownDocument.ts, apps/mobile/src/lib/markdownHtml.test.ts |
markdownHtmlTags advances through ordered code ranges while filtering tags. Tests check HTML image conversion, inline-code preservation, and literal tags after an unclosed fence. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Refactor
Possibly related PRs
- pingdotgg/t3code#24263: Rewrites the same mobile HTML-to-Markdown conversion path and adds conversion tests.
Suggested reviewers: flamboh, juliusmarminge
Merge Risk: 
Minimal · up to 63eac
-
No actionable merge-blocking risk was identified in the changed Markdown conversion path.
Architecture risk: 
Low · up to 0bad2
-
The change affects 1 system.
Changed systems:
apps/mobileArchitecture concerns
No architecture-level concerns identified.Review details
-
Systems and components
- observed — apps/mobile (service) was modified; 2 changed files map to changed impact.
Before / after behavior
- observed — Modified behavior in apps/mobile/src/lib/markdownDocument.ts:
markdownHtmlContentnow advances a range index as tag offsets pass code-range ends and preserves tags when they fall within the current range, replacing the prior per-tag scan of all code ranges. - observed — Modified behavior in apps/mobile/src/lib/markdownHtml.test.ts: Added a test covering 100 lines of inline-code and HTML-image markup: inline tags remain literal, image tags are converted, and trailing text after
<br>is separated by a Markdown line break. - observed — Modified behavior in apps/mobile/src/lib/markdownHtml.test.ts: Added a test asserting that tags after an unclosed HTML fence remain literal, including tags inside a later inline-code range; preceding
<div>markup is converted.
-
Pre-merge checks: 
4
-
Full details: Title check
-
Explanation
The title names the main change: a performance fix in the mobile Markdown conversion helper that scans HTML code ranges once instead of rescanning them for each tag. It follows the conventional commit format and is specific enough for a reviewer scanning history.
Full details: Description check
-
Explanation
The description follows the template. It has Problem, Change, Scope and approval, and Verification sections, and it fills each one with specific content. The Scope section explains why the change qualifies as a small focused fix without a prior issue. The Verification section lists the focused test command and results, a benchmark table, and the measurement limits. The description is long and includes CI and review status details that the template does not request, but the required information is present. The Scope section links no triaged issue or maintainer approval, and it cites issue
#13925only as a separate concern. The template asks for explicit approval for scope, or a clear explanation of why a small obvious fix needs none. The author gives that explanation, so the section is acceptable. The Verification section also says no screenshots were collected, which is reasonable because the change has no visual output. The reproduction steps depend on external links and a commit list, which a reviewer must check.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
Note Written by deepseek v4.1 flash 1017 on behalf of luke. Updated this branch with the latest |
|
Note Written by gpt 6.1 sol on behalf of Luke Resolved the conflict with current main in c871258. Main now filters parsed HTML tags before converting links, so I moved the forward-only code-range cursor into that filter. This preserves main’s anchor/link handling, table-cell handling, and escaping while keeping this PR’s linear scan across code ranges.
|
|
Note Written by gpt 6.1 sol on behalf of Luke Resolved the conflict with main in 63eac02. Both this PR's HTML code-range regressions and main's file-embed tests are retained. Verification against d1f7018: all 36 Markdown HTML/file-embed tests passed, as did both affected package typechecks, formatting, lint, and the first dependency-analysis pass. Checks ran sequentially with one test worker and a 1 GB heap cap. The second dependency-analysis pass remains deferred to CI; native device behavior was not reverified. |
Problem
The mobile HTML-to-Markdown helper checks every HTML tag against
codeRanges.some(...), restarting at the first code range for each tag. Long content containing many inline code examples and HTML image embeds makes that preservation check quadratic, before Markdown parsing or native text layout begins.Change
Advance one range cursor as the HTML replacement callbacks move forward through source offsets. Each expired code range is visited once. Tags inside code remain literal; real HTML images and wrappers outside code retain their existing conversion.
Scope and approval
A small implementation-only performance fix to one mobile Markdown helper: seven production lines. It preserves the current HTML/code conversion behavior and qualifies for the focused small-fix exception. It does not change rendering defaults or claim to solve the separate native text-mount stalls in #13925.
Verification
vp test run apps/mobile/src/lib/markdownHtml.test.ts apps/mobile/src/lib/markdownMedia.test.ts— 28 passed. New cases cover many alternating literal/converted image tags, conversion after the last range, and overlapping ranges following an unclosed fence. Existing HTML entity, attribute, code-fence, screenshot, and media cases pass.Performance comparison
The synthetic document alternates literal
<img>examples inside inline code and real image embeds outside it. Ordered tag offsets allow the preservation check to advance through expired code ranges once, instead of rescanning from the first range for every tag. Existing parsing and conversion rules remain responsible for recognizing the ranges and producing Markdown.Reproduce the measurements
Benchmark script · all raw samples · method, workloads, environment and full commit refs
The script extracts the actual helper source at the named Git commits, strips TypeScript, asserts before/after output equality, warms both versions five times, then takes 30 alternating samples per side. GC runs outside timing. Inputs are reused. Recorded environment: Node v26.10.0 / V8, macOS arm64, Apple M5. The reported median is the upper middle sample; p95 is the recorded 29th ordered sample out of 30.
Exact command and benchmark revisions
Download the linked script and run it from a checkout containing these commits (fetch the three
luke2x/t3codeperformance branches if needed):Arguments are baseline, tree fix, preview fix, and HTML fix. Subsequent
mainmerge commits did not alter any of the three benchmarked helper files; this was checked with Git diffs. The source revisions keep the original measurement reproducible.Measurement limits
These are synthetic production-helper CPU measurements on Node/V8. They exclude caller rendering, native Hermes, native layout, scrolling frames, and heap measurements. No screenshots or native recording were collected because there is no visual-output change being claimed and this host has no native device/simulator available. The focused tests establish output behavior; these timings establish the cost of the isolated helper for the stated fixtures.
CI and review status
As of the 19 October 2026 audit, CI on
8d36084e11passes: 20 successful checks and 7 skipped, with no failing or pending checks. The branch also merges cleanly againstmainat07d38b8d17. Earlier shared settings-mock and web test-dependency failures are historical; they are not current blockers. CI success does not replace maintainer review or establish native performance measurements.Macroscope approved the implementation at its original reviewed commit; subsequent heads include merges from
main. Approval at that SHA is not a claim that all checks or other reviewers have approved the latest head.Implemented and tested with OpenAI Codex (gpt-6.1-sol) in T3 Code. Review follow-ups by DeepSeek V4.1 Flash 1017.