fix(web): disable single-dollar inline math in remark-math#805
Open
heavygee wants to merge 2 commits into
Open
fix(web): disable single-dollar inline math in remark-math#805heavygee wants to merge 2 commits into
heavygee wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Findings
- [Minor] New test imports undeclared markdown packages — the added regression test now imports
unified,remark-parse,remark-rehype, andhast-util-to-htmldirectly, but none of those packages are declared inweb/package.jsonor the root/package manifests (checked repo manifests). The test currently relies on transitive dependencies from other markdown packages, so a valid dependency update can remove or move them and makebun run test:webfail even though this test still compiles conceptually. Evidence:web/src/components/assistant-ui/markdown-text.test.ts:2
Suggested fix:{ "devDependencies": { "hast-util-to-html": "^9.0.5", "remark-parse": "^11.0.0", "remark-rehype": "^11.1.2", "unified": "^11.0.5" } }
Summary
- Review mode: initial
- One dependency hygiene issue found in the new test. No correctness issue found in the
remarkMathoption change itself. Residual risk: I could not run the web test locally becausebunis not installed in this runner.
Testing
- Not run (automation):
buncommand not available in runner.
HAPI Bot
| @@ -1,7 +1,11 @@ | |||
| import { describe, expect, it } from 'vitest' | |||
| import { unified } from 'unified' | |||
There was a problem hiding this comment.
[Minor] This test now imports unified, remark-parse, remark-rehype, and hast-util-to-html directly, but the web package does not declare any of them. That makes bun run test:web depend on transitive dependencies from other markdown packages, so a valid dependency update can break this test without touching it.
Suggested fix:
{
"devDependencies": {
"hast-util-to-html": "^9.0.5",
"remark-parse": "^11.0.0",
"remark-rehype": "^11.1.2",
"unified": "^11.0.5"
}
}
heavygee
added a commit
to heavygee/hapi
that referenced
this pull request
Jun 6, 2026
The new markdown-text regression test imports these directly to drive the unified pipeline end-to-end. They were resolving via transitive deps from remark-math and rehype-katex, which is fragile — a future dep upgrade can remove the transitives and break the test. Declare them explicitly under devDependencies. No code change; lockfile records the same versions that were already installed transitively (unified@11.0.5, remark-parse@11.0.0, remark-rehype@11.1.2, hast-util-to-html@9.0.5). Addresses the HAPI Bot review finding on PR tiann#805. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Findings
- No issues found.
Summary
- Review mode: follow-up after new commits
- The previous dependency hygiene finding is resolved by declaring the direct test dependencies in
web/package.jsonandbun.lock. I reviewed the full current diff and found no additional correctness, security, regression, performance, or maintainability issues with theremarkMathoption change or added coverage. Residual risk: I could not execute the web tests in this runner becausebunis not installed.
Testing
- Not run (automation):
buncommand not available in runner.
HAPI Bot
heavygee
added a commit
to heavygee/hapi
that referenced
this pull request
Jun 6, 2026
Extend hapi-pr-status to recognise both Codex surfaces in use: - Upstream tiann/hapi: GHA codex-pr-review.yml posts formal reviews as github-actions[bot]; clean signal is body matching /No findings| No high-confidence|No issues found|No actionable/. Existing behavior. - Fork heavygee/hapi: cloud auto-review (ChatGPT subscription, no API key) posts issue comments as chatgpt-codex-connector[bot]; clean signal is body matching /Codex Review:.*Didn.t find any/. Surface is auto-detected from repo owner - no flag needed. Either surface ALSO honors the cold-review-clean label as an explicit operator override (e.g. when Codex flagged a P1 already addressed in a follow-up commit and the operator doesn't want to wait for re-review). Treat 'skipping' check-bucket as not-a-failure (was previously caught by the catch-all FAIL branch - wrong; SKIPPED is legitimate, not broken). Extend hapi-pr-create with the actual fork-stage gate (was previously documented as 'planned'). Before opening upstream PR: 1. Find fork PR for current branch via gh pr list --head <branch> 2. Run hapi-pr-status against it 3. Refuse upstream PR if status is not clean Bypass via --skip-fork-stage flag or HAPI_PR_CREATE_NO_FORK_STAGE=1 env (for trivial typo/doc-only changes where bot review adds nothing). When no fork PR exists, hapi-pr-create emits the exact gh pr create incantation operator should use to open one (base=main, draft). Verified end-to-end against PR #30 (test PR for the gate itself): - Codex commented "Codex Review: Didn't find any major issues" at T+~4min after @codex review trigger - hapi-pr-status detected the clean comment automatically - cold-review-clean label applied as manual practice run, also detected via the operator-override path - PR #30 closed, branch (fix/markdown-single-dollar-katex) is upstream-bound via tiann#805 Doc section 3.1.5 added to repo-layout-and-dev-flow.md documenting the dual-signal design and the upstream vs fork surface difference. Co-authored-by: Cursor <cursoragent@cursor.com>
Default remark-math configuration treats $...$ as inline LaTeX, which turns any prose containing two currency amounts (e.g. "save $400 vs the $200 plan") into a KaTeX block — paragraphs collapse, whitespace is stripped, the running text is re-rendered as math symbols. Pass `singleDollarTextMath: false` to remarkMath so single $ is plain text. Block math `$$...$$` (on its own line) still renders, matching GitHub-flavored markdown semantics. Single source of truth: MARKDOWN_PLUGINS is shared by MarkdownText, Reasoning, and MarkdownRenderer — fix lands in all three surfaces. Adds 3 regression tests that drive the unified pipeline end-to-end: prose with multiple "$N" amounts produces no `class="katex"` and no `<math>` element; `$$...$$` block math still does. Co-authored-by: Cursor <cursoragent@cursor.com>
The new markdown-text regression test imports these directly to drive the unified pipeline end-to-end. They were resolving via transitive deps from remark-math and rehype-katex, which is fragile — a future dep upgrade can remove the transitives and break the test. Declare them explicitly under devDependencies. No code change; lockfile records the same versions that were already installed transitively (unified@11.0.5, remark-parse@11.0.0, remark-rehype@11.1.2, hast-util-to-html@9.0.5). Addresses the HAPI Bot review finding on PR tiann#805. Co-authored-by: Cursor <cursoragent@cursor.com>
669322f to
f4e9735
Compare
There was a problem hiding this comment.
Findings
- No issues found.
Summary
- Review mode: follow-up after new commits
- I reviewed the full current diff for PR #805, including the
remarkMathoption change, the markdown pipeline regression coverage, and the added direct test dependencies. No correctness, security, regression, performance, or maintainability issues found. Residual risk: I could not run tests locally becausebunis not installed in this runner; the PR body reportsbun run testandbun run typecheckpassing.
Testing
- Not run (automation):
buncommand not available in runner.
HAPI Bot
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Disables single-dollar inline math in
remark-mathso prose containing currency amounts no longer collapses into KaTeX output.$$...$$block math (on its own line) is unchanged.Fixes #800.
Problem
remark-mathis configured with defaults, which meanssingleDollarTextMath: true. Any pair of$characters in a paragraph becomes a math span, so messages like:render with everything between the first and last
$collapsed into a single<span class="katex">block — whitespace stripped, letters reflowed as italic math symbols, apostrophes turned into prime marks. The text is no longer readable as prose.Currency mentions are common in chat (pricing discussions, plan comparisons, billing), so the false-positive rate is high enough that day-to-day prose has been hitting it.
Approach
Pass
singleDollarTextMath: falsetoremarkMathinweb/src/components/assistant-ui/markdown-text.tsx. This is a single source of truth —MarkdownText,Reasoning, andMarkdownRendererall import the sameMARKDOWN_PLUGINSarray, so the fix lands in every markdown surface at once.Block math
$$...$$continues to render. This matches GitHub-flavored markdown semantics, which only treat$$...$$(or fenced```math) as math.Trade-off vs #237
#237 added KaTeX support and explicitly mentioned single-dollar form
$ E= mc^2 $as desired. This PR partially reverses that — single-dollar inline math goes away. Math-heavy users will need to switch to$$E = mc^2$$on its own line. The trade-off felt worth it because false positives from currency$are common in chat prose, while inline math is rare; happy to revisit if maintainers prefer a different policy.Testing
3 new regression tests in
web/src/components/assistant-ui/markdown-text.test.tsdrive the unified pipeline end-to-end and assert against the rendered HTML:$Namounts → noclass="katex", no<math>element, all amounts present as literal text$$E = mc^2$$block math → still produces a KaTeX blockRelated
Questions
Open to a different policy if maintainers prefer (e.g. only disable single-dollar when followed by a digit, to keep
$ E= mc^2 $working). Happy to iterate.