refactor(ui): adopt Astryx 0.6.2 components.math for Markdown math - #5543
Conversation
654a39a to
44bd16d
Compare
jackwener
left a comment
There was a problem hiding this comment.
Seat: kabi-opus-review-orchestrator. Review of exact head 44bd16ddf269be5c7fe691da7f351a3fc68060c2. Draft, MERGEABLE. Hosted test and package had no conclusion at lock — I am not writing green. COMMENT only.
Verdict
No P0–P2. 1×P3. The version move is consistent everywhere it has to be, the patch rewrite applies cleanly against the real 0.6.2 package, and the KaTeX rendering contract is carried over unchanged rather than reopened.
Topology
The recorded base d9c67042332a23fadc61a791b17a7cfb47babd80 is not an ancestor of this head. The merge-base is 7baf8b053122860e62840ded44923b95186f5e4f; behind main by 2. Use the merge-base to diff this branch.
What I verified
Version consistency — packages/ui, apps/desktop, root package.json, and package-lock.json all say 0.6.2 for @astryxdesign/core (plus cli and theme-neutral); the patch file is renamed to @astryxdesign+core+0.6.2.patch with no 0.6.1 file left behind; LICENSE and THIRD_PARTY_NOTICES.txt track both the version and the patch filename. Nothing is left pointing at the old version.
The patch actually applies. npm install at this head reports @astryxdesign/core@0.6.2 ✔. That matters more than reading the patch: a rebased 1,724-line patch against a bumped upstream is exactly where a silent partial application would hide.
No new HTML sink. The KaTeX call moved from the old plugin into MarkdownMath, and the options are byte-identical — output: 'htmlAndMathml', strict: 'warn', throwOnError: false, and critically trust: false, which is what keeps \href / \url / \includegraphics out. The dangerouslySetInnerHTML is the same pre-existing one, now in one place instead of two.
The new patch hunk is scoped correctly. trimStreamingArtifacts now skips [, *, ~~ that fall inside a completed inline math span. I checked that this actually covers this repo's syntax: translateMarkdownMath normalises \(…\) → $…$ and \[…\] / $$…$$ → $$…$$ before upstream sees the text, and escapes bare $ to \$, so upstream's dollar-based mathRanges scanner sees exactly the spans this repo means. The patches/README.md entry carries a removal condition, as the other entries do.
543/543 @maka/ui tests pass on a clean rebuild at this head.
P3 — every inline formula leaves an invisible U+200B in the rendered text
inlineMathSource (packages/ui/src/markdown-math.tsx:242-250) appends a zero-width space after every inline formula:
return `${prefix}$${escapeFormulaDollars(formula)}$${ZWSP}`;The prefix is conditional — it is added only when the preceding character is a digit or $, i.e. only when something could actually glue. The suffix is unconditional.
Probe on the production render path at this head (notes/probes/pr-5543-zwsp-44bd16ddf.mjs, SHA-256 c9c1b7bd6248e4047d52e5d3e2ecc2af01090caba8be15aac8462ef551dba9b4), counting U+200B in the rendered markup:
| source | U+200B in output |
|---|---|
the value \(x\) is fine |
1 — next char is a space, nothing to separate |
the value \(x\) |
1 — end of line, nothing follows at all |
v2\(x + 1\)3 done |
2 — both genuinely needed |
before \[x + 1\] after |
0 |
plain text only |
0 |
So prose containing any inline math now yields invisible characters on copy. The usual costs: pasting a snippet into a shell or code, and in-page find across the boundary.
ZWSP is introduced by this PR — it does not appear in markdown-math.tsx on the merge-base.
Suggested fix: make the suffix conditional on the following source character the same way the prefix is conditional on the preceding one. The call site has source and delimited.end, so the lookahead is available. One caveat on that suggestion: at the streaming tail the next character genuinely is not known yet, so the conditional needs to keep emitting the ZWSP while the tail is still growing, and drop it once the following character has arrived.
未验证
- Hosted
testandpackagewere not terminal at lock; the 543 above is my local run. - No browser and no Electron — the probe is server-rendered markup, so I did not check how a real selection or clipboard copy behaves, only that the character is in the DOM.
- I did not review the 1,724-line patch hunk by hunk; I verified it applies, and read the one hunk this PR adds.
- I did not diff rendered math output against 0.6.1 for visual regressions.
简体中文
结论:无 P0–P2,1×P3。
拓扑:PR 记录的 base d9c670423 不是本 head 的祖先;merge-base 是 7baf8b053,落后 main 2 个。
已核:版本一致性(三个 package.json + lockfile + 补丁文件名 + LICENSE + 第三方声明全部到 0.6.2,没有残留 0.6.1);补丁确实能打上 —— 本 head 上 npm install 报 @astryxdesign/core@0.6.2 ✔(1724 行补丁跟着上游升版重排,正是"悄悄只打上一半"最容易藏的地方,所以这条比读补丁更重要);没有新增 HTML 注入面 —— KaTeX 调用只是从旧插件搬进 MarkdownMath,选项逐字未变,trust: false 保留;新增的补丁 hunk 作用域正确 —— 我确认了 translateMarkdownMath 会把 \(…\) 归一成 $…$、\[…\]/$$…$$ 归一成 $$…$$,并把裸 $ 转义成 \$,所以上游基于美元符的 mathRanges 扫描正好覆盖本仓库的语法;543/543 UI 测试通过(本 head 干净重建)。
P3 —— 每个行内公式都会在渲染结果里留下一个不可见的 U+200B。 inlineMathSource(markdown-math.tsx:242-250)无条件在公式后面加零宽空格,而前缀是有条件的(仅当前一个字符是数字或 $)。探针实测:the value \(x\) is fine 后面是空格也会留 1 个;the value \(x\) 行尾什么都没有也会留 1 个;只有 v2\(x+1\)3 那种才真的需要。后果是含行内公式的正文一复制就带上不可见字符(粘进终端或代码、页内查找跨边界都会出问题)。ZWSP 是本单引入的,merge-base 上没有。
修法:让后缀像前缀一样,根据后一个字符决定是否添加 —— 调用点有 source 和 delimited.end,能前瞻。但有个前提:流式的尾部确实还不知道下一个字符,所以尾部仍在增长时要继续加,等后一个字符到了再去掉。
未验证:锁定时 test/package 未终态(543 是我本地跑的);无浏览器无 Electron,只确认字符在 DOM 里,没验真实选中/剪贴板行为;1724 行补丁我没有逐 hunk 审,只验了能打上并读了本单新增那一处;没有和 0.6.1 做渲染视觉回归对比。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
|
Follow-up on the same head
Why this matters more than a red checkThe story is added by this PR ( Meanwhile the same delimiters do produce I flagged "no browser, no Electron" as uncovered in my review above. This is that gap paying out, so I am not treating it as someone else's problem: my verdict was formed without the evidence that contradicts it. Where I would look first, stated as a hypothesis rather than a finding
The decisive experiment is cheap: make the assertion wait (poll or await a frame) before counting. If it then finds 6, the defect is the assertion's timing and the feature is fine. If it still finds 0, math genuinely does not render in the browser bundle and this is a P1 on the PR's headline behaviour. I have not run the browser story myself, so I am not grading it until that is answered. Not approving at this head. 简体中文撤回上面「无 P0-P2」这一半结论 ——
为什么这比「CI 红了」更重要:那个 story 是本单新增的,它的 我在上面的评论里把「无浏览器、无 Electron」列为未覆盖项。这正是那个缺口兑现了 —— 我的结论是在缺少这条证据的情况下下的,所以由我自己撤回。 先看哪里(这是假设,不是结论): 验证很便宜:让断言先等待(轮询或等一帧)再计数。若变成 6,缺陷在断言时序,能力本身没问题;若仍是 0,那就是浏览器打包路径下数学真的不渲染,这是本单核心行为上的 P1。 我没有自己跑过浏览器 story,所以在这个问题回答之前不定级。本 head 不批准。
|
44bd16d to
93c9958
Compare
|
Addressed in What changed
Probe results at this head (U+200B in rendered markup): Coverage added: a no-ZWSP assertion for benign neighbors, an adjacent-formula pair test, and a streaming test appending a digit chunk after a committed closer (75/75 ui tests pass). |
0.6.2 adds an opt-in Markdown math renderer (components.math), a TextInput IME fix for CJK input, ChatToolCalls a11y announcements, and a localized Spinner label. Upstream reworked Markdown.js parse plumbing for the math option (mathParseOptions / hasMathRenderer cache reset), so the settledText / transformSource patch hunks were re-applied by hand onto the new shape; all other patched files were byte-identical between 0.6.1 and 0.6.2. Generated-by: Devin
93c9958 to
be05cef
Compare
Replace the private-use TOKEN transport with Astryx 0.6.2's official math pipeline: components.math renders parsed math nodes through the same KaTeX configuration, and transformSource now translates the host delimiters into upstream grammar instead of smuggling tokens past the parser. \(…\) inline math is emitted as `$…$` guarded by zero-width spaces so adjacent digits or dollars cannot trip upstream's currency checks, and `\[…\]`/`$$…$$` emit `$$` lines that repeat the enclosing quote/list margin so containers still own them. Bare `$` outside code and link destinations is escaped, preserving the "shell variables and currency stay literal" contract that upstream's bare-$ pairing would otherwise break. Display math on a table-row line degrades to an inline span so `$$` lines never split a row. The regenerated patch adds one upstream fix: trimStreamingArtifacts' unclosed-marker scans ([, *, ~~) now skip completed inline math spans; previously a last-line formula containing those characters was trimmed or auto-closed as Markdown. Generated-by: Devin
be05cef to
d483863
Compare
|
Fixed in Reproduced locally against this branch's storybook-static with the same The Separately addressed on the same push: the bump commit now also maps |
jackwener
left a comment
There was a problem hiding this comment.
Seat: kabi-opus-review-orchestrator. Re-review at exact head d483863b4f820ece12004331c6dd4b83e47ad0c3. Draft; test and package had no conclusion at lock — not writing green. COMMENT only.
Restoring the verdict I withdrew: back to no P0–P2, and the P3 is now fixed
Two things were open after my withdrawal. Both are answered.
The storybook failure was assertion timing, not a broken feature
I posed it as a two-way question — wait before counting, and either it finds 6 (timing) or still 0 (the feature does not render in the browser). The answer is the first, and the fix says so in the story:
// Math nodes land on a later parser pass than the first commit, so the
// count must wait rather than query synchronously.
await waitFor(() => {
expect(canvasElement.querySelectorAll('.maka-math-inline')).toHaveLength(6);
expect(canvasElement.querySelectorAll('.maka-math-display')).toHaveLength(2);
});The literal-dollar assertions are kept. So the browser does render the math; the previous play simply measured before the parser's later pass.
My ZWSP P3 is fixed, and fixed better than I proposed
I suggested making the trailing guard conditional on the following character, and flagged that the streaming tail cannot look ahead. The implementation answers that more cleanly than my suggestion did: at the source tail the span is not committed at all — markSafe() is skipped, so the next chunk rescans from the opener and decides the closing guard with the neighbour in hand. That avoids emitting a guard that later turns out to be wrong, rather than emitting one defensively.
prepareMarkdownMath now threads cache.text[textStart - 1] through as priorTextChar so the incremental path still knows the preceding character across a chunk boundary, which is what keeps the opening guard correct on resume.
The comment now states the rule plainly: "The guards only exist when the neighbor is actually hostile; an unconditional one would leave invisible characters in copied text."
Verified on the production render path at this head (notes/probes/pr-5543-zwsp-guard-d483863b4.mjs, SHA-256 1f9952f87fe75f9a470e59134b36e835ea9c934bbfa51c93daf10ac771a6104d), counting U+200B and math spans:
| source | U+200B | math spans |
|---|---|---|
the value \(x\) is fine |
0 | 1 |
the value \(x\) |
0 | 1 |
the value \(x\), ok |
0 | 1 |
the value \(x\)y done |
0 | 1 |
the value \(x\)3 done |
1 | 1 |
v2\(x + 1\) done |
1 | 1 |
v2\(x + 1\)3 done |
2 | 1 |
\(a\)\(b\) done |
1 | 2 |
plain text only |
0 | 0 |
Benign neighbours are clean; the guard still fires for a hugging digit on either side and for an immediately adjacent span, which is what nextBreaksInlineClose is for. Math still renders in every case. 546/546 @maka/ui tests pass on a clean rebuild at this head, with @astryxdesign/core@0.6.2 confirmed installed before running.
One question, deliberately not graded
The story's own comment says math nodes land on a later parser pass than the first commit. Between those two passes the translated source is present as text, so the reader would briefly see $x$ rather than rendered math.
I cannot tell from here whether that is perceptible, whether it predates this PR, or whether the streaming cursor already covers it — patches/README.md describes keeping host syntax behind that cursor, which suggests the streaming path does. This is the same browser-side area where my earlier verdict went wrong, so I am asking rather than concluding: is there a visible flash of raw $…$ on first paint for non-streamed content, and if so was it there before?
未验证
testandpackagewere not terminal at lock; the 546 is my local run.- No browser and no Electron. That includes the flash question above — I have not looked at a real first paint.
- I did not re-verify the version consistency, LICENSE/notices or patch application at this head; those were checked at
44bd16ddfand the delta since does not touch them (97044a1b8reorganises the dependency bump into its own commit,d483863b4is the refactor).
简体中文
恢复我先前撤回的结论:回到无 P0–P2,而且那条 P3 已修。
storybook 失败是断言时序,不是能力坏了。 我给的是二选一:先等待再计数,要么变 6(时序)、要么仍是 0(浏览器里真不渲染)。答案是前者,修法自己写了注释:数学节点落在首次提交之后的某一轮解析,所以要 waitFor 而不是同步查询。字面美元符的断言保留了。
我那条 ZWSP P3 已修,而且比我提的更好。 我建议让尾部守卫依后一个字符决定,并指出流式尾部无法前瞻;实现给的答案更干净:在源码尾部干脆不提交这个 span(跳过 markSafe()),下一块从开标记重扫,拿到邻居再决定闭合守卫 —— 这是「不发出可能是错的守卫」,而不是「先防御性地发一个」。prepareMarkdownMath 另外把 cache.text[textStart-1] 作为 priorTextChar 传下去,保证跨块恢复时开头那个守卫仍然判对。注释现在把规则写明了:守卫只在邻居确实有敌意时才存在,无条件的守卫会在复制出来的文本里留下不可见字符。
探针实测(见表):良性邻居全部 0 个 ZWSP;数字贴边(左/右)与紧邻另一个公式时守卫仍然触发;每种情况数学都照常渲染。546/546 UI 测试通过(干净重建,且跑之前确认已装 0.6.2)。
一个只问不定级的问题:story 的注释说数学节点落在首次提交之后的某一轮解析。那两轮之间,翻译后的源码是以文本形式存在的,读者会短暂看到 $x$ 而不是渲染后的公式。我在这里判断不了它是否可感知、是否早于本单就存在、或者流式游标是否已经覆盖了它(patches/README.md 描述的正是把宿主语法留在游标后面,看起来流式路径是覆盖的)。这正是我先前判断出错的那个浏览器侧区域,所以我只问不下结论:非流式内容首帧会不会闪一下原始 $…$?如果会,这是本单引入的还是本来就有?
未验证:锁定时 test/package 未终态(546 是我本地跑的);无浏览器无 Electron —— 包括上面那个闪烁问题,我没看过真实首帧;本 head 未重核版本一致性/LICENSE/补丁应用,那些在 44bd16ddf 上核过,此后的增量没有触及。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
jackwener
left a comment
There was a problem hiding this comment.
Approving at exact head d483863b4f820ece12004331c6dd4b83e47ad0c3. This binds to that SHA only.
Why
No P0–P2 at this head. The P3 I raised — an unconditional zero-width space after every inline formula — is fixed, and fixed in a stronger form than I proposed: rather than guessing the guard at the source tail, the span is left uncommitted so the next chunk decides it with the neighbour in hand.
The earlier Storybook smoke failure resolved to assertion timing, not a broken feature: play now waits instead of counting synchronously, and the literal-dollar assertions are retained.
CI at the moment of approval, on this SHA: test COMPLETED/SUCCESS (13:23:12Z), package COMPLETED/SUCCESS (13:20:36Z), every other check success or skipped. MERGEABLE. The head has not moved since I reviewed it. Still marked draft — flagging that, not treating it as a blocker, since the approval was requested on the no-P0–P2 condition.
Open and not closed by this approval
I asked one question that has not been answered, and approving does not answer it: the story's comment says math nodes land on a later parser pass than the first commit, so between those passes the translated source exists as text. Whether a reader briefly sees raw $…$ on first paint for non-streamed content — and whether that predates this PR — is unresolved. It is not a P0–P2 on any evidence I have, so it does not block under the stated condition, but it should not vanish at merge.
What this approval rests on, and what it does not cover
Verified locally at this head: packages/ui/dist deleted and rebuilt, with @astryxdesign/core@0.6.2 confirmed installed before anything was run; 546/546 @maka/ui tests pass; a 9-case probe on the production render path confirms benign neighbours now emit zero U+200B while a hugging digit or an adjacent span still gets the guard, with math rendering in every case.
Verified at the previous head 44bd16ddf and unchanged since: version consistency across all three manifests, the lockfile, the patch filename, LICENSE and THIRD_PARTY_NOTICES.txt; and that the rewritten 1,724-line patch actually applies (@astryxdesign/core@0.6.2 ✔).
Not covered: no browser and no Electron — including the first-paint question above, which I have not looked at. No visual regression comparison of rendered math against 0.6.1. I did not review the patch hunk by hunk; I verified it applies and read the one hunk this PR adds. I am not making a merge decision.
If the head moves, this approval does not carry over.
简体中文
批准绑定 exact head d483863b4,只对这一个 SHA 有效。
理由:本 head 无 P0–P2。我提的那条 P3(每个行内公式后无条件加零宽空格)已修,且比我的建议更强 —— 不是在源码尾部猜那个守卫,而是让该 span 不提交,由下一块拿到邻居后决定。先前 Storybook smoke 的失败查明为断言时序而非能力损坏:play 改为等待后再计数,字面美元符断言保留。
批准时该 SHA 上:test SUCCESS(13:23:12Z)、package SUCCESS(13:20:36Z),其余检查均成功或跳过;MERGEABLE;自我评审以来 head 未动。仍标记为 draft —— 我指出这一点,但不当作阻塞,因为批准是按「无 P0-P2」这个条件请求的。
批准不关闭的开放问题:我问过而尚未有答案的一条 —— story 注释说数学节点落在首次提交之后的某一轮解析,那两轮之间翻译后的源码以文本存在。非流式内容首帧是否会闪一下原始 $…$、以及是否早于本单就存在,仍未解决。以我手上的证据它不构成 P0–P2,故在既定条件下不阻塞,但不应随合并消失。
批准依据:本 head 本地删除并重建 packages/ui/dist,跑任何东西之前先确认已装 @astryxdesign/core@0.6.2;546/546 UI 测试通过;9 例探针在生产渲染路径上确认良性邻居零 U+200B、数字贴边与紧邻公式仍触发守卫、每例数学均渲染。上一 head 44bd16ddf 上核过且此后未变:三个清单/lockfile/补丁文件名/LICENSE/第三方声明的版本一致性,以及重排后的 1724 行补丁确实能打上(✔)。
不覆盖:无浏览器、无 Electron —— 包括上面那个首帧问题,我没看过;没有与 0.6.1 的渲染视觉回归对比;补丁未逐 hunk 审(只验了能打上并读了本单新增那一处);合并与否不归我。head 一漂,本批准即不覆盖。
Automated review notice: This approval was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one. Seat: kabi-opus-review-orchestrator.
Summary
Two commits: bump
@astryxdesign/*0.6.1 → 0.6.2, then move Markdown math onto the new officialcomponents.mathrenderer and delete the private-use TOKEN transport that shuttled formulas past the parser.Maka's delimiter contract (
\(…\)inline,\[…\]/$$…$$display, bare$literal) predates upstream math support and intentionally rejects bare$…$— it protects shell variables, currency, and prose. Astryx parses only$…$/$$…$$, sotransformSourcenow translates delimiters into upstream grammar instead of smuggling tokens:\(…\)→$…$with zero-width-space guards so adjacent digits/$cannot trip upstream's currency checks (the trailing guard is unconditional — a committed span must survive later stream chunks)\[…\]/$$…$$→$$lines carrying the enclosing quote/list margin, so> \[…\]and- \[…\]stay inside their container; on a table-row line they degrade to an inline span rather than splitting the row$outside code, link destinations, autolinks, and reference definitions →\$, so prose dollars stay literalcomponents.mathrenders each node through the same KaTeX config (htmlAndMathml,strict: 'warn',throwOnError: false,trust: false) and the samemaka-math/maka-math-inline|displayDOM contractThe dependency patch also gains one upstream fix:
trimStreamingArtifactsnow skips[,*, and~~inside completed$…$spans — previously a last-line formula containing them (e.g.\left[y\right],x * z) was trimmed or auto-closed as Markdown mid-stream.TOKEN machinery,
MARKDOWN_MATH_PLUGINS, and the literal-token re-entry path are removed; the incremental cache stays because the transform still needs tail-incremental scanning.A new
TranscriptMathstory (Product/Markdown) shows the contract end to end — inline/display math alongside literal$HOME,$5–$10,$x$, and`\(x\)`— with aplayfunction asserting the span counts and literal text.Verification
npm --workspace @maka/ui run build;markdown-body,streaming-text,markdown-rhythm-contract,markdown-han-script,streaming-display-redaction,attachment-imagesuites — 72 tests pass$$…$$,$in link destinations, digit-glued\(…\), formulas with\$, math inside table cellsnpm --workspace @maka/desktop run typecheck(incl.tsconfig.storybook.json),typecheck:stories,build-storybooknpm run format,npm run lint,npm run astryx:surface-inventory,npm run astryx:theme(regenerated theme committed in the bump commit), third-party notices checksTranscriptMathrendered frommain's storybook-static (left) and this branch's (right) — same story, same viewport; output is pixel-identical:AI use
Tool(s) and scope: Devin — implementation, upstream patch rework, tests, story.
Checklist
Does this PR entail a change in behavior?