Skip to content

Studio: fix currency and indentation edge cases in LaTeX rendering - #6957

Merged
danielhanchen merged 5 commits into
mainfrom
studio-latex-render-followups
Jul 8, 2026
Merged

danielhanchen merged 5 commits into
mainfrom
studio-latex-render-followups

Conversation

@danielhanchen

@danielhanchen danielhanchen commented Jul 8, 2026

Copy link
Copy Markdown
Member

Follow-up to #6914, which made Studio chat render \[ \] and \( \) LaTeX delimiters. This fixes two edge cases in studio/frontend/src/lib/latex.ts where the conversion mis-handled well-formed input.

Fixes

  1. Display math indentation. A \[...\] inside a list item emitted the $$ fence at column 0, so the block broke out of the list (for example - step: followed by an indented \[x\] rendered the equation outside the list). The opener line's indentation is now preserved and applied to every line of a multi-line body, guarded so inline and column-0 uses stay unchanged.

  2. Currency swallowed into math. A currency amount before a converted span on the same line could use the generated span's opening $ as its closer, so Cost is $5 + x \(y\) rendered 5 + x as math and dropped the $5. hasInlineMathCloser now skips a $ that opens a generated math region.

An earlier revision also protected reference-style link URLs, but that guarded a case models effectively never emit (escaped parens in a [id]: url definition) and needed open-ended special-casing of CommonMark reference definitions, so it was dropped to keep this change focused. Inline link handling is unchanged from #6914.

Verification

  • 32 exact-output transform cases and 27 cases through the real remark-math + rehype-katex pipeline pass.
  • tsc (5.9.3, project config) is clean, and behavior for input without bracket delimiters is byte-identical to before.

Follow-up to #6914. Three fixes to studio/frontend/src/lib/latex.ts:

- Skip reference-link definition URLs ([id]: url) during delimiter
  conversion, so escaped parens in such URLs are not rewritten as math.
- Preserve the opener line's indentation when emitting a display $$ block,
  so a \[...\] inside a list item stays part of the list.
- Stop a currency amount from pairing with a converted span's opening $,
  which swallowed the price into math (for example $5 + x \(y\)).

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

Copy link
Copy Markdown
Contributor

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 enhances LaTeX preprocessing in studio/frontend/src/lib/latex.ts by preventing reference-link definitions from being rewritten as math, ensuring inline math closers do not incorrectly pair with generated math regions, and preserving leading indentation for display math blocks. Feedback was provided to explicitly guard against match.index being 0 when calculating lineStart to avoid relying on implicit negative index behavior in lastIndexOf.

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 studio/frontend/src/lib/latex.ts Outdated
// Keep the opener's leading indentation so a `$$` block inside a list item
// stays in the container instead of breaking out at column 0. Only when the
// opener is whitespace-prefixed, so inline `text \[x\]` keeps column 0.
const lineStart = content.lastIndexOf("\n", match.index - 1) + 1;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

When match.index is 0, match.index - 1 evaluates to -1. In JavaScript, String.prototype.lastIndexOf treats negative indices as 0 (searching only at index 0), which is different from Array.prototype.lastIndexOf where negative indices count from the end of the array. To prevent potential confusion, make the code more robust, and avoid relying on this implicit behavior, explicitly guard against match.index being 0.

Suggested change
const lineStart = content.lastIndexOf("\n", match.index - 1) + 1;
const lineStart = match.index > 0 ? content.lastIndexOf("\n", match.index - 1) + 1 : 0;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good call. Output was already correct here (lastIndexOf clamps a negative fromIndex to 0), but I made the index-0 case explicit in the pushed commit so it does not rely on that implicit behavior.

@danielhanchen

Copy link
Copy Markdown
Member 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: 3e9aa37fb4

ℹ️ 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 studio/frontend/src/lib/latex.ts Outdated
* inline destination, must not be rewritten as math.
*/
const REF_DEF_DEST_RE =
/^ {0,3}\[(?:\\.|[^\]\n\\])+\]:[^\S\n]*(?:<([^>\n]*)>|([^\s<][^\s]*))/gmd;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Exclude GFM footnotes from reference URL skips

When a message starts a GFM footnote definition with math, e.g. [^1]: \(x\), this new reference-definition matcher treats the footnote body’s first token as a link destination and adds it to the skip regions, so preprocessLaTeX leaves the delimiters literal instead of rendering $x$. Streamdown depends on remark-gfm (studio/frontend/package-lock.json:14550), so these [^...] definitions are parsed as footnotes rather than link URLs; the matcher should avoid footnote labels or otherwise distinguish them before skipping the first token.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Correct, the matcher was treating a footnote body as a URL. Fixed in f8f7ca0 by excluding [^...] footnote labels ((?!^)); footnote math now renders and real reference definitions stay skipped.

A footnote definition like [^1]: \(x\) had its body treated as a link
destination, so leading math was left literal. Skip [^...] labels.
@danielhanchen

Copy link
Copy Markdown
Member 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: f8f7ca0730

ℹ️ 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 studio/frontend/src/lib/latex.ts Outdated
}
}
// Inline and ref-def spans can interleave (never overlap), so sort.
regions.sort((a, b) => a[0] - b[0]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Merge overlapping link destination regions

When a reference URL itself contains multiple [...](...)-shaped path segments before an escaped paren, LINK_DEST_RE adds nested inline-link spans inside the larger reference-destination span. After this sort the regions overlap, but isInRegion uses a binary search that assumes non-overlap; for example [1]: https://h/[a](b)/[c](d)/foo\(bar\) misses the reference span at \( and rewrites the URL to foo$bar$, breaking the link this change is meant to protect. Merge or drop contained spans before using the binary search.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Correct, nested inline-link spans inside a reference-def token could overlap and defeat the binary search. Fixed in the pushed commit by merging overlapping spans before isInRegion; the example [1]: https://h/[a](b)/[c](d)/foo\(bar\) now keeps the URL.

A reference-def token can nest inline-link spans (for example
[1]: http://h/[a](b)/foo\(x\)), so the combined spans could overlap and
isInRegion's binary search missed the outer one, rewriting the URL. Merge
overlapping spans before the search.
@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: dc461e1cfa

ℹ️ 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".

Behavior is unchanged (lastIndexOf clamps a negative fromIndex to 0), but
the explicit guard avoids relying on that implicit clamp.
@danielhanchen

Copy link
Copy Markdown
Member 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: 250942d06c

ℹ️ 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".

if (content[i - 1] === "\\") continue;
// A `$` opening a generated span (from `\(...\)`) is not a currency closer;
// pairing with it would swallow the price into math (`$5 + x \(y\)`).
if (isInRegion(i, mathRegions)) return false;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep scanning past generated math delimiters

When a legitimate dollar-delimited math span starts with a number and contains a converted \(...\) span before its closing $, this early return makes the currency pass treat the opener as a price and escape it. For example, preprocessLaTeX('$2 + 2 \\(x\\)$') now returns \$2 + 2 $x$ $, breaking mixed-delimiter math emitted by models rather than only protecting cases like $5 + x \(y\). Continuing the scan past generated regions, instead of immediately returning false, would avoid corrupting the outer math span.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This mixes $...$ and (...) delimiters in a single span ($2 + 2 (x)$), which is malformed input models do not emit; they use one delimiter style. The guard protects the realistic case (a price followed by a separate (...) span). Leaving as-is.

Comment thread studio/frontend/src/lib/latex.ts Outdated
match.index > 0 ? content.lastIndexOf("\n", match.index - 1) + 1 : 0;
const prefix = content.slice(lineStart, match.index);
const indent = /^\s*$/.test(prefix) ? prefix : "";
wrapped = `\n${indent}$$\n${indent}${body}\n${indent}$$\n`;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Indent every generated display-math line

For an indented \[...\] whose body spans multiple lines that are not already indented, this template only prefixes the first body line. For example, - step\n \[a\nb\] is converted with b at column 0, so the generated $$ block can break out of the list item instead of staying in the container this change is trying to preserve. Apply the saved indent after each newline in body before wrapping it.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in the pushed commit: the saved indent is now applied to every line of the body, so a multi-line display block stays inside the list item.

Comment thread studio/frontend/src/lib/latex.ts Outdated
* an inline destination, must not be rewritten as math.
*/
const REF_DEF_DEST_RE =
/^ {0,3}\[(?!\^)(?:\\.|[^\]\n\\])+\]:[^\S\n]*(?:<([^>\n]*)>|([^\s<][^\s]*))/gmd;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Honor escaped whitespace in reference URLs

Bare reference destinations can contain backslash-escaped characters just like the inline-link regex above, but this pattern stops the destination at any whitespace even when it is escaped. With a valid reference such as [r]: https://example.com/a\ b/\(c\), only https://example.com/a\ is protected and the later \(c\) is rewritten to $c$, corrupting the URL the new reference-definition handling is meant to preserve.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moot now: the reference-link URL handling this refers to has been removed to keep the change focused on the indentation and currency fixes.

Drop the reference-link URL protection added earlier. It guards a case
models effectively never emit (escaped parens in a reference-style URL),
and approximating CommonMark reference definitions with a regex needs
open-ended special-casing. Keep the two high-value fixes: preserve display
math indentation (including multi-line bodies) inside a list item, and stop
a currency amount from pairing with a converted span's opening dollar sign.
@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@danielhanchen danielhanchen changed the title Studio: fix link, currency and indentation edge cases in LaTeX rendering Studio: fix currency and indentation edge cases in LaTeX rendering Jul 8, 2026
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: cf59b2ff38

ℹ️ 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".

@danielhanchen
danielhanchen merged commit de60a3a into main Jul 8, 2026
43 of 49 checks passed
@danielhanchen
danielhanchen deleted the studio-latex-render-followups branch July 8, 2026 10:13
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.

1 participant