Repository navigation
fix(shared): preserve code shielding and titled Markdown links - #30665
ElliotDrel wants to merge 9 commits into
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: Would Approve Macroscope's review found this PR approvable — This adjusts Markdown thread-link handling to preserve code shielding and leave titled links unchanged. The reported code-span label corruption remains subject to the separate correctness blocking gate; no additional material risk requires human judgment. Needs human approval because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c8ebdf10c
ℹ️ 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".
|
Note 🤖 OpenAI assistant responding on behalf of Elliot P2: Preserve a whole link when its label contains a literal unmatched backtick. At The scanner rejects the whole-link match, then matches its href as a bare URL. Relabeling to I reproduced this with the exact scanner/replacement logic extracted from this head in Node 24; I did not run the repository test suite. The balanced-code fix works, but a thread title with a lone backtick still corrupts a copied link. Preserve the whole candidate when the unmatched label run cannot actually form a cross-boundary code span, while retaining the new shielding cases. Add a regression for a lone literal backtick with no later matching delimiter. |
|
Note Written by Verified in a real dev environmentreproduced titled-link corruption on the original pr head fixed that case but still corrupted lone-backtick labels. further checks found bracketed/escaped titles, trailing title whitespace, and a later code opener following a literal backtick. reproduced each in the real composer and with failing rows in existing tests. pushed scoped corrections to this pr's existing head branch without force. final head: after restarting the corrected app, verified:
main, before: corrected pr, matching input: paste checks and navigation, including the additional title and backtick cases: real provider result: Not verifiednative ios/android clients, electron shell behavior, remote/tunnel or multiple-environment navigation, and other providers were not exercised in this linux web pass. the full hosted test/build suite was not rerun here; scoped blacksmith checks passed. no merge, rebase, force push, or label changes were performed. three independent-review rounds found the additional cases above. the last round approved the preceding head in one review and found trailing title whitespace in the other. that finding was reproduced, corrected, and checked; the final whitespace correction has not had another independent review because the review-round budget was reached. this is runtime verification, not a merge-readiness claim. |
|
Note 🤖 gpt-6.1-sol responding on behalf of Elliot Added your current-head runtime report to the description with attribution and retained its native/remote and hosted-suite limits. I also inspected the final 38ed00b whitespace-only diff: the title group now consumes trailing horizontal whitespace and the existing table covers double-quoted, single-quoted and parenthesized titles. No additional finding from that narrow source review; it is not a repeated runtime pass or merge-readiness claim. No further code change is needed from this report. |
The label scan paired backtick runs twice: once inside each label, then again across the line to tell a cross-boundary opener from a literal backtick. Both answer the same question, so a single map from each run to its same-length closer on the line now serves both. A label skips spans that close inside it and yields to one that closes past it. Behavior is unchanged: a 300k-input differential fuzz against the prior scanner matched exactly, and adversarial 1 MB inputs stay linear. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>




What Changed and Why
Issue: Follow-up to #30588 for #30576, targeting
fix/issue-30576. Thread URLs inside CommonMark code could be rewritten, and mobile collected references then relabeled the same Markdown with two full scans/parses per render.Why this change was necessary: Keep remark-parse's block/code correctness while avoiding repeated parsing for title updates. The existing bounded matcher still controls link acceptance, title exclusion and source offsets.
New status quo:
prepareThreadLinksstores exact source and accepted link records, without retaining the AST or resolved titles. Reference collection, replacement and relabeling accept that result while preserving existing string callers. Mobile memoizes one current preparation on[markdown]; reference resolution uses the current environment and relabeling uses current titles. Changed source prepares again. No global cache, history, dependency, renderer refactor, provider or contract changes. Web/desktop composer string callers preserve their behavior. Changed source still incurs a CommonMark parse when gated code/link candidates require it; storage is proportional to source and accepted links.Verification
c2175fc5bd79fed6928c37048e999287ac7e13e1by gpt-6.1-sol through the Codex harness, Windows / Node 26.11.1, lockfile-matched scoped dependencies.tsc --noEmit, targeted lint, scoped formatting andgit diff --checkpass. These packages expose no separate build script. No repo-wide checks, browser, server, provider or native/Hermes execution.0.30447 / 0.11238 / 0.00078; 1,088 B1.36146 / 0.63635 / 0.00944; 9,520 B15.13115 / 6.39087 / 0.07199. Changed-source paths append an iteration value; unchanged-source uses one prepared result. This measures shared functions, not React rendering, native performance or precise allocation. No Hermes claim.dd5f98eeecad5db5b10314296221df115f130fbd; reviewed its base diff. Continued existingf45a87b508cabd5453693f15fc315b20621436d3ancestry under the explicit continuity exception, with no main merge or force push. Hosted checks/reviews for this new head are pending; historical green runs below do not validate it.After merge
Parent #30588 remains at
a364059587521142bf7324d2a1e32f9510a58155and lacks upstream budget reduction9e85538535c8553901351112ab02dd1dd62db5ee(Claude/Codex/MCP 20/20/40). The inherited Agent Context Budget failure is separate from this parser change. Parent owner @julius must integrate the coherent upstream change and verify its budgets before the follow-up inherits it normally. No generated-budget edit, label bypass, unrelated prompt change or main merge was made. Owner follow-up is already scheduled; no additional ping or watch started.Implemented by gpt-6.1-sol through the Codex harness.
Superseded verification and historical evidence; not current-head validation
What Changed and Why
Issue: Follow-up to #30588 for #30576, targeting
fix/issue-30576. Thread URLs inside valid multiline code spans were collected and rewritten ataa2a4bba628983f4b4c3a9da25bee2495c35356b.Why this change was necessary: The regex excluded line endings and the closer map reset on each newline. Regex fences also ignored block context and could consume unequal backtick runs. Extending that scanner correctly would require block parsing. The shared package already depends on
remark-parseandunified; this reuses their CommonMark parser and the parser-owned-offset approach inmarkdownMathRanges, adding no dependency.New status quo: Lazily collect code/inline-code source ranges, then filter the existing bounded link candidates against them. Balanced code wholly inside a link label remains part of that link. Titles (including escaped delimiters, brackets and trailing horizontal whitespace) remain excluded. Collection and replacement share the same matcher; untouched source slices are preserved without AST serialization. URL and code-syntax gates avoid parsing ordinary messages. Web, desktop and mobile consume this shared behavior; provider adapters/contracts are unchanged.
Verification
Implementing agent at
f45a87b508cabd5453693f15fc315b20621436d3: lockfile-matched scoped dependencies; 87/87 parser/context tests pass. New controls cover multiline spans, complete unequal runs, inline versus block fences, quoted/nested-length fences, paragraph boundaries and live links. Existing regressions remain intact.Before correction, both multiline regressions failed collection; direct execution of
aa2a4bbaalso confirmed relabeling changed the code content. After correction, the same inputs yield no references and remain unchanged.Shared
tsc --noEmit, targeted lint, formatting andgit diff --checkpass. Shared has no separate build script; no repository-wide checks were run.Local Node 26 timing (100 warmed calls): plain 1.4 KB text ~0.001 ms/call; one live link ~0.018 ms; short multiline code/link ~0.197 ms; dense 8.8 KB/140-paragraph code/link input ~40.079 ms. Full parsing costs CPU/allocation on gated inputs: the dense sample showed ~9.4 MB end-of-loop heap growth and ~66 KB retained after GC, not a precise allocation measurement. This trades that cost for existing CommonMark block correctness rather than a second block parser.
Named-group
RegExp.execremains in use for Hermes compatibility; no new regex syntax. Hermes/native clients and current-head browser/provider behavior were not executed. Historical runtime evidence below does not validate this head. Hosted checks/reviews are pending at publication.Implemented by gpt-6.1-sol through the Codex harness.
Independent performance and base readiness assessment
Assessed by gpt-6.1-sol through the Codex harness.
Historical evidence (superseded implementation descriptions and statuses; not current-head validation)
Additional exact-head runtime verification reported by Maria at
38ed00b5bd03ade8bda915523e2bce3a0edbd8a9: #30665 (comment) . Her Linux real-web/Codex pass reproduced main's titled-link corruption and checked titles, escaped delimiters, trailing horizontal whitespace, balanced/literal backticks, code shielding, chip navigation, and intact provider context after corrections. She reports 76 focused parser/context tests and scoped checks passing. I inspected the report and final whitespace diff; I did not repeat her runtime pass. Native clients and remote/multi-environment behavior remain unverified by that pass. Prior head 38ed00b had 27 passing hosted checks; both inline findings remain resolved. That historical status does not establish checks or approval for later commits.Simplification at
aa2a4bba628983f4b4c3a9da25bee2495c35356bOpus 5.5 removed duplicated backtick pairing without changing tests or public behavior. It reports 300,000 generated-input comparisons matching the preceding head and scoped typecheck/lint/format checks passing. The primary agent independently reviewed the source diff and reran both focused parser/context files: 75/75 tests pass. Generated-input comparison establishes equivalence to the previous implementation, not complete Markdown correctness. Native-client verification was not repeated. Hosted CI now passes at this head: aggregate Check, lint, build/typecheck, package and web tests, all eight server shards, agent-context budget and native fingerprint comparison succeed. Two other fingerprint check runs were cancelled and seven platform/preview checks skipped; none are pending. Both existing review threads are resolved. The PR is clean and mergeable, awaiting owner incorporation; prior-head runtime evidence above remains attributed and historical. Existing limitations around the regex's handling of unequal backtick runs and fence precedence remain unchanged.
Simplified by Claude Opus 5.5 through T3 Code; independently reviewed and tested by gpt-6.1-sol through the Codex harness.