feat: extract @tale/markdown package + fix code-block line numbers#1692
Merged
Conversation
Carve markdown rendering out of @tale/webui into a new @tale/markdown workspace. Web, docs, and platform now share one Shiki singleton, one component set, and one styling pass. Streaming primitives (incremental reveal, mid-stream syntax repair, paginated tables, CJK plugin) move in alongside the static surface as opt-in subpath exports. Bug fixes - code-block: countLines() and Shiki now tokenise the same string so trailing-newline blocks no longer drop the last line number. - mermaid: rejected lazy-load promise no longer poisons subsequent renders; stale temp DOM fragments are removed on unmount so error artefacts don't accumulate when revisiting parse-error charts. - HeaderCopyButton: pending reset timer cleared on unmount. - routed-markdown: mailto:/tel:/protocol-relative hrefs stay plain anchors instead of leaking into the router. - cards: unknown Lucide icon names render nothing (validated against iconNames) so the flex gap doesn't open a phantom slot. Visual polish - Row-hover tint on every Shiki .line, edge-to-edge. - Sticky table thead with row-hover at <tr> level + scoped headers. - Expanded eager Shiki languages (39 grammars: rust, go, java, kotlin, swift, c/c++/c#, ruby, php, elixir, lua, scala, scss, sql, toml, ini, graphql, prisma, hcl, zig, …); rare grammars lazy-load on demand. - Brighter dark-mode fg-muted/fg-subtle so body prose reads cleanly. - Themed body bg in markdown storybook so the iframe canvas inverts with addon-themes. - CodeGroup panels share the exact same HighlightedCode body as CodeBlock — single border, no double-padding ring. Storybook - New @tale/markdown storybook on port 6008 with dedicated stories for every export: Markdown, CodeBlock, CodeGroup, Callout (5 tones), Mermaid (flowchart/sequence/state/pie/parse-error), Tabs, Steps, Cards, Accordion, Frame, AnchoredHeading, plus formatting/headings samples and an IncrementalMarkdown reveal demo. - Tailwind now actually runs in storybook: missing postcss.config.mjs added to packages/ui, packages/webui, packages/markdown, and the react-package plop template. - Storybook turbo task added; platform moved off the duplicate :6006 port to :6009 so ui + platform can run side-by-side. Wiring - @tale/markdown added to root workspaces and to docs/web/platform deps. - @tale/webui drops markdown subpath exports + the matching deps (react-markdown, remark-gfm, rehype-raw, shiki, mermaid). - services/docs swaps Markdown → RoutedMarkdown so SPA navigation survives the migration; bare Markdown stays router-agnostic. - services/platform shiki + markdown-types collapse to thin re-exports of the shared package.
…ault Streaming and static prose now share the exact same component map — including the static CodeBlock, AnchoredHeading, callouts, tabs, accordions — so streaming previews look identical to a finished message. countCursors matched every aria-hidden element so the hover-anchor indicator inside AnchoredHeading was miscounted as a second cursor; tightening the selector to .animate-cursor-blink removes the false positive and lets us drop the heading shim entirely.
Contributor
📝 WalkthroughWalkthroughThis PR extracts the markdown pipeline from Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/markdown/src/anchored-heading.tsx (2)
135-136:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace hardcoded English strings with translation keys.
The
aria-labeluses hardcoded English strings ("Link copied" and "Copy link to this section"), which violates the coding guidelines. As per coding guidelines: "No hardcoded user-facing strings in React — always use the translation hook; a stray English literal in JSX is a bug."🌐 Proposed fix using translation hook
Assuming a translation hook is available in this package context:
+import { useT } from '@/lib/i18n'; // adjust import path as needed + export function AnchoredHeading({ level, className, children, }: AnchoredHeadingProps) { + const { t } = useT('markdown'); const explicit = extractExplicitId(children); const renderedChildren = explicit.children; const id = explicit.id ?? slugifyHeading(renderedChildren); const Tag = level; const [copied, setCopied] = useState(false); // ... handleCopy implementation return ( <Tag id={id} className={`group scroll-mt-24 ${className}`}> <a href={`#${id}`} className="text-fg-base no-underline"> {renderedChildren} </a> <button type="button" onClick={handleCopy} - aria-label={copied ? 'Link copied' : 'Copy link to this section'} + aria-label={copied ? t('heading.linkCopied') : t('heading.copyLink')} aria-live="polite"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/markdown/src/anchored-heading.tsx` around lines 135 - 136, The aria-label currently contains hardcoded English ("Link copied"/"Copy link to this section"); replace these with translation keys by using the project's translation hook (e.g., import and call useTranslations or useI18n at the top of the AnchoredHeading component) and use the returned t function for aria-label: map copied ? t('anchoredHeading.linkCopied') : t('anchoredHeading.copyLinkToSection') (or similar keys), keeping the existing copied state and the aria-live attribute unchanged; ensure you add the new translation keys to the locale files so both messages are present.
114-125:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winTimer leak: pending reset not cleared on unmount.
The
setTimeoutat line 121 is not canceled if the component unmounts before the 1500ms delay completes. This is flagged in the PR objectives as a fixed bug ("HeaderCopyButton timer leak: pending reset timer cleared on unmount"), but the cleanup is missing.🔧 Proposed fix to clear timer on unmount
export function AnchoredHeading({ level, className, children, }: AnchoredHeadingProps) { const explicit = extractExplicitId(children); const renderedChildren = explicit.children; const id = explicit.id ?? slugifyHeading(renderedChildren); const Tag = level; const [copied, setCopied] = useState(false); const handleCopy = async () => { // Guard for SSR / non-browser environments where `navigator` is absent. if (typeof window === 'undefined' || !navigator.clipboard) return; const url = `${window.location.origin}${window.location.pathname}#${id}`; try { await navigator.clipboard.writeText(url); setCopied(true); - setTimeout(() => setCopied(false), 1500); + const timer = setTimeout(() => setCopied(false), 1500); + return () => clearTimeout(timer); } catch (error) { console.warn('[anchored-heading] clipboard write failed', error); } };Alternatively, use
useEffectwith cleanup:export function AnchoredHeading({ level, className, children, }: AnchoredHeadingProps) { const explicit = extractExplicitId(children); const renderedChildren = explicit.children; const id = explicit.id ?? slugifyHeading(renderedChildren); const Tag = level; const [copied, setCopied] = useState(false); + + useEffect(() => { + if (!copied) return; + const timer = setTimeout(() => setCopied(false), 1500); + return () => clearTimeout(timer); + }, [copied]); const handleCopy = async () => { // Guard for SSR / non-browser environments where `navigator` is absent. if (typeof window === 'undefined' || !navigator.clipboard) return; const url = `${window.location.origin}${window.location.pathname}#${id}`; try { await navigator.clipboard.writeText(url); setCopied(true); - setTimeout(() => setCopied(false), 1500); } catch (error) { console.warn('[anchored-heading] clipboard write failed', error); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/markdown/src/anchored-heading.tsx` around lines 114 - 125, The handleCopy function sets a 1500ms timeout to reset setCopied(true) but never clears that timer on unmount; capture the timeout id (e.g., via a ref like copyResetTimerRef) when calling setTimeout inside handleCopy and add a useEffect cleanup that clears the pending timer with clearTimeout(copyResetTimerRef.current) (and nulls the ref) so the pending reset is cancelled on unmount; update references to the timer id in handleCopy and ensure any previous timer is cleared before creating a new one.packages/markdown/src/components/accordion.stories.tsx (1)
41-41:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
role="list"for Safari/VoiceOver semantics.Tailwind CSS v4 with Preflight resets
list-styletonone, which causes Safari/VoiceOver to remove list semantics. Based on learnings, explicitrole="list"is required for WCAG 2.1 Level AA compliance with VoiceOver.♿ Proposed fix
- <ul className="list-disc pl-5"> + <ul className="list-disc pl-5" role="list">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/markdown/src/components/accordion.stories.tsx` at line 41, The unordered list element <ul className="list-disc pl-5"> in accordion.stories.tsx is losing list semantics in Safari/VoiceOver due to Tailwind Preflight; add role="list" to that <ul> (the <ul className="list-disc pl-5"> element) so it reads <ul className="list-disc pl-5" role="list"> to restore proper accessibility semantics for VoiceOver/WCAG 2.1 AA.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/markdown/src/code-block.tsx`:
- Around line 101-102: The aria-labels in the CodeBlock component use hardcoded
English strings ("Copied" / "Copy code"); replace them with localized strings by
calling the translation function or injected labels (e.g., use t('copied') and
t('copyCode') or props.copyLabel/props.copiedLabel) instead of literals, update
any comparisons against the English literal 'Copied' to compare against the
translated value (from the same t(...) or prop) and keep aria-live="polite"
unchanged; refer to the CodeBlock component and the copied boolean to locate
where to swap the strings for translated keys.
In `@packages/markdown/src/components/code-group.tsx`:
- Around line 85-86: Replace the hardcoded user-facing strings in the CodeGroup
component: move the aria-label="Code examples" and the fallback tab title "Tab
N" into the translation layer (or accept localized props) instead of literals;
use the component's translation hook (e.g., call to t(...) or a passed-in prop)
to supply the aria-label and to generate the fallback tab title (e.g.,
t('code.examples') for the aria-label and t('tab_n', { n: index + 1 }) or a
localized format for the tab label) and update any defaultProps or prop types
accordingly so all user-visible strings in CodeGroup come from i18n rather than
hardcoded English.
In `@packages/markdown/src/highlighted-code.tsx`:
- Around line 115-121: Replace the hardcoded ARIA labels in the copy button (the
aria-label currently using "Copied" and "Copy code") with values from the
translation hook used in this component (e.g., call the component's i18n
translator like t(...) or useTranslation().t(...)); update the aria-label
expression inside the button that references copied to use the translated
strings instead, and add corresponding translation keys (e.g., "copy.copied" and
"copy.copyCode") to the component's translation namespace so the text is
localized. Ensure you reference the same translator instance used elsewhere in
this file/component and keep the conditional (copied ? ... : ...) logic intact.
- Around line 96-101: The handleCopy function starts a timeout (setTimeout(...,
1500)) but never clears it on unmount, risking stale state updates; fix by
storing the timeout ID in a ref (e.g., copyTimeoutRef) and add a useEffect
cleanup that calls clearTimeout(copyTimeoutRef.current) on unmount, and also
clear any existing timeout before setting a new one inside handleCopy so
setCopied(false) cannot run after unmount; reference the handleCopy function and
setCopied state to locate where to add the ref and cleanup.
In `@packages/markdown/src/routed-markdown.tsx`:
- Around line 32-62: RoutedAnchor currently sends relative hrefs like
"relative.md" or "?q=1" into the TanStack <Link> (in function RoutedAnchor),
which TanStack Router doesn't resolve as relative; update the href detection to
treat non-absolute, non-root-relative hrefs as plain anchors: extend the checks
around NON_INTERNAL_HREF_RE, isHttpExternal, and isHash to also consider a href
string without a leading "/" (and not starting with "./" or "../" if you want to
allow explicit relative paths) or starting with "?" as a plain <a> (keep
target/_blank logic only for http(s)), and only pass absolute root-relative
paths (starting with "/") to <Link to={...}>. Ensure you reference RoutedAnchor,
NON_INTERNAL_HREF_RE, isHttpExternal, isHash, and the <Link> return path when
making the change.
In `@packages/markdown/src/shiki.ts`:
- Around line 117-151: The current check in highlightCode compares code.length
(UTF-16 units) to MAX_SHIKI_BYTES, which misrepresents actual byte size; change
the comparison to use the UTF-8 byte length by computing const byteLen = new
TextEncoder().encode(code).length and compare byteLen > MAX_SHIKI_BYTES, or
alternatively rename MAX_SHIKI_BYTES to MAX_SHIKI_CHARS and keep the existing
char-based check; update the MAX_SHIKI_BYTES constant name/usages and any
comments accordingly so the symbol (MAX_SHIKI_BYTES) and function
(highlightCode) reflect the chosen meaning.
- Around line 181-207: Validate and restrict resolvedLang before using it in the
dynamic import: in the block that checks loaded.includes(resolvedLang) and calls
highlighter.loadLanguage(... import(`shiki/langs/${resolvedLang}.mjs`) ...), add
a whitelist/validation step (e.g., allow only a safe token regex like
/^[a-z0-9_-]+$/ or check membership against a known list of shiki language ids)
and bail out to the plaintext fallback (same behavior as the existing catch) if
validation fails; reference resolvedLang, resolveLanguage, and
highlighter.loadLanguage when locating the code to change.
In `@packages/markdown/src/streaming/incremental-markdown.tsx`:
- Line 50: The double-cast on STREAMING_BASE (const STREAMING_BASE =
baseComponents as unknown as MarkdownComponentMap) must be removed; either
change the declaration/type of baseComponents to MarkdownComponentMap where it's
created or add a small adapter function (e.g.,
convertComponentsToMarkdownComponentMap) that accepts react-markdown Components
and returns a properly typed MarkdownComponentMap, then assign STREAMING_BASE to
that adapter's result; update references to baseComponents and STREAMING_BASE so
the types align without using `as unknown as`.
In `@packages/markdown/test/setup.ts`:
- Around line 14-16: Replace the double type assertion assignment to
globalThis.ResizeObserver with Vitest's vi.stubGlobal API: locate the usage of
MockResizeObserver and the globalThis.ResizeObserver assignment and change it to
call vi.stubGlobal('ResizeObserver', MockResizeObserver) so you no longer use
`as unknown as typeof ResizeObserver` and the global is properly stubbed for
tests.
---
Outside diff comments:
In `@packages/markdown/src/anchored-heading.tsx`:
- Around line 135-136: The aria-label currently contains hardcoded English
("Link copied"/"Copy link to this section"); replace these with translation keys
by using the project's translation hook (e.g., import and call useTranslations
or useI18n at the top of the AnchoredHeading component) and use the returned t
function for aria-label: map copied ? t('anchoredHeading.linkCopied') :
t('anchoredHeading.copyLinkToSection') (or similar keys), keeping the existing
copied state and the aria-live attribute unchanged; ensure you add the new
translation keys to the locale files so both messages are present.
- Around line 114-125: The handleCopy function sets a 1500ms timeout to reset
setCopied(true) but never clears that timer on unmount; capture the timeout id
(e.g., via a ref like copyResetTimerRef) when calling setTimeout inside
handleCopy and add a useEffect cleanup that clears the pending timer with
clearTimeout(copyResetTimerRef.current) (and nulls the ref) so the pending reset
is cancelled on unmount; update references to the timer id in handleCopy and
ensure any previous timer is cleared before creating a new one.
In `@packages/markdown/src/components/accordion.stories.tsx`:
- Line 41: The unordered list element <ul className="list-disc pl-5"> in
accordion.stories.tsx is losing list semantics in Safari/VoiceOver due to
Tailwind Preflight; add role="list" to that <ul> (the <ul className="list-disc
pl-5"> element) so it reads <ul className="list-disc pl-5" role="list"> to
restore proper accessibility semantics for VoiceOver/WCAG 2.1 AA.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9e80e934-f2e9-438d-8523-624097fbef21
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (85)
.gitignore.vscode/extensions.jsonknip.config.tspackage.jsonpackages/markdown/.oxlintrc.jsonpackages/markdown/.storybook/main.tspackages/markdown/.storybook/manager.tspackages/markdown/.storybook/preview.tsxpackages/markdown/package.jsonpackages/markdown/postcss.config.mjspackages/markdown/src/anchored-heading.stories.tsxpackages/markdown/src/anchored-heading.tsxpackages/markdown/src/code-block.stories.tsxpackages/markdown/src/code-block.tsxpackages/markdown/src/components/accordion.stories.tsxpackages/markdown/src/components/accordion.tsxpackages/markdown/src/components/callout.stories.tsxpackages/markdown/src/components/callout.tsxpackages/markdown/src/components/cards.stories.tsxpackages/markdown/src/components/cards.tsxpackages/markdown/src/components/code-group.stories.tsxpackages/markdown/src/components/code-group.tsxpackages/markdown/src/components/frame.stories.tsxpackages/markdown/src/components/frame.tsxpackages/markdown/src/components/mermaid.stories.tsxpackages/markdown/src/components/mermaid.tsxpackages/markdown/src/components/registry.tsxpackages/markdown/src/components/steps.stories.tsxpackages/markdown/src/components/steps.tsxpackages/markdown/src/components/tabs.stories.tsxpackages/markdown/src/components/tabs.tsxpackages/markdown/src/extract-toc.test.tspackages/markdown/src/extract-toc.tspackages/markdown/src/formatting.stories.tsxpackages/markdown/src/globals.csspackages/markdown/src/globals.css.d.tspackages/markdown/src/headings.stories.tsxpackages/markdown/src/highlighted-code.tsxpackages/markdown/src/markdown.stories.tsxpackages/markdown/src/markdown.tsxpackages/markdown/src/plugins/__tests__/micromark-cjk-attention.test.tspackages/markdown/src/plugins/micromark-cjk-attention.tspackages/markdown/src/reading-time.tspackages/markdown/src/routed-markdown.tsxpackages/markdown/src/shiki.tspackages/markdown/src/streaming/__tests__/find-block-split.test.tspackages/markdown/src/streaming/__tests__/incremental-markdown.test.tsxpackages/markdown/src/streaming/__tests__/normalize-html-blocks.test.tspackages/markdown/src/streaming/__tests__/remend-markdown.test.tspackages/markdown/src/streaming/find-block-split.tspackages/markdown/src/streaming/incremental-markdown.stories.tsxpackages/markdown/src/streaming/incremental-markdown.tsxpackages/markdown/src/streaming/normalize-html-blocks.tspackages/markdown/src/streaming/remend-markdown.tspackages/markdown/src/types.tspackages/markdown/test/setup.tspackages/markdown/tsconfig.jsonpackages/markdown/vitest.config.tspackages/ui/postcss.config.mjspackages/ui/src/globals.csspackages/webui/package.jsonpackages/webui/postcss.config.mjspackages/webui/src/index.tspackages/webui/src/markdown/code-block.stories.tsxpackages/webui/src/markdown/code-block.tsxpackages/webui/src/markdown/components/frame.tsxpackages/webui/src/markdown/markdown.stories.tsxpackages/webui/src/markdown/shiki.tsservices/docs/Dockerfileservices/docs/app/components/docs/docs-toc.tsxservices/docs/app/pages/docs-page.tsxservices/docs/package.jsonservices/platform/Dockerfileservices/platform/app/features/chat/components/canvas/__tests__/canvas-pane.test.tsxservices/platform/app/features/chat/components/canvas/canvas-code-renderer.tsxservices/platform/app/features/chat/components/message-bubble/__tests__/code-block.test.tsxservices/platform/app/features/chat/components/message-bubble/code-block.tsxservices/platform/app/features/chat/components/typewriter-text.tsxservices/platform/app/features/documents/components/document-preview-text.tsxservices/platform/lib/utils/markdown-types.tsservices/platform/lib/utils/shiki.tsservices/platform/package.jsonservices/web/Dockerfiletools/plop/templates/react-package/postcss.config.mjsturbo.json
💤 Files with no reviewable changes (5)
- packages/webui/src/markdown/components/frame.tsx
- packages/webui/src/markdown/code-block.stories.tsx
- packages/webui/src/markdown/markdown.stories.tsx
- packages/webui/src/markdown/code-block.tsx
- packages/webui/src/markdown/shiki.ts
Comment on lines
+101
to
+102
| aria-label={copied ? 'Copied' : 'Copy code'} | ||
| aria-live="polite" |
Contributor
There was a problem hiding this comment.
Replace hardcoded ARIA copy labels with translated labels.
Lines 101–102 introduce hardcoded English strings in user-facing accessibility text. Please route these through the translation layer (or injected localized props from caller).
As per coding guidelines: "Every user-facing string goes through the translation layer — never compare against an English literal in code, tests, stories, or comments."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/markdown/src/code-block.tsx` around lines 101 - 102, The aria-labels
in the CodeBlock component use hardcoded English strings ("Copied" / "Copy
code"); replace them with localized strings by calling the translation function
or injected labels (e.g., use t('copied') and t('copyCode') or
props.copyLabel/props.copiedLabel) instead of literals, update any comparisons
against the English literal 'Copied' to compare against the translated value
(from the same t(...) or prop) and keep aria-live="polite" unchanged; refer to
the CodeBlock component and the copied boolean to locate where to swap the
strings for translated keys.
Comment on lines
85
to
+86
| aria-label="Code examples" | ||
| className="border-border-base flex gap-1 border-b px-2 pt-2" | ||
| className="border-border-base flex items-stretch border-b" |
Contributor
There was a problem hiding this comment.
Localize tablist label and fallback tab title.
Line 85 ("Code examples") and Line 146 ("Tab N") are user-facing English literals in JSX. These should come from the translation layer (or localized props) to keep CodeGroup i18n-compliant.
As per coding guidelines: "No hardcoded user-facing strings in React — always use the translation hook; a stray English literal in JSX is a bug."
Also applies to: 146-146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/markdown/src/components/code-group.tsx` around lines 85 - 86,
Replace the hardcoded user-facing strings in the CodeGroup component: move the
aria-label="Code examples" and the fallback tab title "Tab N" into the
translation layer (or accept localized props) instead of literals; use the
component's translation hook (e.g., call to t(...) or a passed-in prop) to
supply the aria-label and to generate the fallback tab title (e.g.,
t('code.examples') for the aria-label and t('tab_n', { n: index + 1 }) or a
localized format for the tab label) and update any defaultProps or prop types
accordingly so all user-visible strings in CodeGroup come from i18n rather than
hardcoded English.
Comment on lines
+96
to
+101
| const handleCopy = async () => { | ||
| try { | ||
| await navigator.clipboard.writeText(normalisedCode); | ||
| setCopied(true); | ||
| setTimeout(() => setCopied(false), 1500); | ||
| } catch (error) { |
Contributor
There was a problem hiding this comment.
Clear the copy reset timer on unmount to avoid stale state updates.
Line 100 starts a timeout but this component never clears it on unmount. Add a ref + cleanup effect (same pattern used elsewhere in this PR).
Suggested patch
import { useEffect, useMemo, useState } from 'react';
+import { useRef } from 'react';
@@
const [html, setHtml] = useState<string | null>(null);
const [copied, setCopied] = useState(false);
+ const copyResetTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
@@
+ useEffect(() => {
+ return () => {
+ if (copyResetTimerRef.current) clearTimeout(copyResetTimerRef.current);
+ };
+ }, []);
+
const handleCopy = async () => {
@@
await navigator.clipboard.writeText(normalisedCode);
setCopied(true);
- setTimeout(() => setCopied(false), 1500);
+ if (copyResetTimerRef.current) clearTimeout(copyResetTimerRef.current);
+ copyResetTimerRef.current = setTimeout(() => setCopied(false), 1500);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/markdown/src/highlighted-code.tsx` around lines 96 - 101, The
handleCopy function starts a timeout (setTimeout(..., 1500)) but never clears it
on unmount, risking stale state updates; fix by storing the timeout ID in a ref
(e.g., copyTimeoutRef) and add a useEffect cleanup that calls
clearTimeout(copyTimeoutRef.current) on unmount, and also clear any existing
timeout before setting a new one inside handleCopy so setCopied(false) cannot
run after unmount; reference the handleCopy function and setCopied state to
locate where to add the ref and cleanup.
Comment on lines
+115
to
+121
| <button | ||
| type="button" | ||
| onClick={handleCopy} | ||
| disabled={copied} | ||
| aria-label={copied ? 'Copied' : 'Copy code'} | ||
| aria-live="polite" | ||
| className={cn( |
Contributor
There was a problem hiding this comment.
Localize copy button ARIA text instead of hardcoded English.
Lines 119–120 hardcode user-facing labels ("Copied", "Copy code"). This breaks the repo i18n rule and ships untranslated accessibility text.
As per coding guidelines: "No hardcoded user-facing strings in React — always use the translation hook; a stray English literal in JSX is a bug."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/markdown/src/highlighted-code.tsx` around lines 115 - 121, Replace
the hardcoded ARIA labels in the copy button (the aria-label currently using
"Copied" and "Copy code") with values from the translation hook used in this
component (e.g., call the component's i18n translator like t(...) or
useTranslation().t(...)); update the aria-label expression inside the button
that references copied to use the translated strings instead, and add
corresponding translation keys (e.g., "copy.copied" and "copy.copyCode") to the
component's translation namespace so the text is localized. Ensure you reference
the same translator instance used elsewhere in this file/component and keep the
conditional (copied ? ... : ...) logic intact.
Comment on lines
+32
to
+62
| function RoutedAnchor({ href, children }: ComponentPropsWithoutRef<'a'>) { | ||
| const isExternal = | ||
| typeof href === 'string' && NON_INTERNAL_HREF_RE.test(href); | ||
| const isHttpExternal = | ||
| typeof href === 'string' && | ||
| (href.startsWith('http://') || href.startsWith('https://')); | ||
| const isHash = typeof href === 'string' && href.startsWith('#'); | ||
|
|
||
| if (!href || isExternal || isHash) { | ||
| return ( | ||
| <a | ||
| href={href} | ||
| target={isHttpExternal ? '_blank' : undefined} | ||
| rel={isHttpExternal ? 'noopener noreferrer' : undefined} | ||
| className={ROUTER_LINK_CLASS} | ||
| > | ||
| {children} | ||
| </a> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <Link | ||
| // oxlint-disable-next-line typescript/no-explicit-any -- runtime-typed router target | ||
| to={href as any} | ||
| className={ROUTER_LINK_CLASS} | ||
| > | ||
| {children} | ||
| </Link> | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
LGTM — comprehensive scheme/hash detection.
NON_INTERNAL_HREF_RE correctly catches every scheme: form plus //host, so mailto:, tel:, data:, etc., all stay plain anchors and only http(s) get target=_blank — matches the PR's "no router leak" fix. The internal-link case correctly falls through to <Link>.
One minor consideration: a relative href like relative.md or ?q=1 will reach <Link to=...>, which TanStack Router doesn't natively resolve as relative. If authored markdown ever produces such hrefs, consider treating "no leading /" as plain-anchor too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/markdown/src/routed-markdown.tsx` around lines 32 - 62, RoutedAnchor
currently sends relative hrefs like "relative.md" or "?q=1" into the TanStack
<Link> (in function RoutedAnchor), which TanStack Router doesn't resolve as
relative; update the href detection to treat non-absolute, non-root-relative
hrefs as plain anchors: extend the checks around NON_INTERNAL_HREF_RE,
isHttpExternal, and isHash to also consider a href string without a leading "/"
(and not starting with "./" or "../" if you want to allow explicit relative
paths) or starting with "?" as a plain <a> (keep target/_blank logic only for
http(s)), and only pass absolute root-relative paths (starting with "/") to
<Link to={...}>. Ensure you reference RoutedAnchor, NON_INTERNAL_HREF_RE,
isHttpExternal, isHash, and the <Link> return path when making the change.
Comment on lines
+117
to
+151
| /** | ||
| * Cap on the input size we'll synchronously tokenize on the main thread. | ||
| * Above this, callers should fall back to a plain-text render — Shiki's | ||
| * `codeToHtml` is O(n) but blocking, and on a 250 KB document the freeze | ||
| * runs 300 ms-2 s on a mid-range laptop. | ||
| */ | ||
| export const MAX_SHIKI_BYTES = 64_000; | ||
|
|
||
| export interface HighlightResult { | ||
| html: string; | ||
| language: string; | ||
| } | ||
|
|
||
| type ShikiTheme = 'light' | 'dark' | 'github-light' | 'github-dark'; | ||
|
|
||
| function normalizeTheme(theme: ShikiTheme): 'github-dark' | 'github-light' { | ||
| return theme === 'dark' || theme === 'github-dark' | ||
| ? 'github-dark' | ||
| : 'github-light'; | ||
| } | ||
|
|
||
| /** | ||
| * Tokenize `code` into highlighted HTML. Returns `null` when: | ||
| * - `code.length` exceeds `MAX_SHIKI_BYTES` (caller should plain-text) | ||
| * - the underlying highlighter fails to initialize or render | ||
| * | ||
| * Languages outside the eager list are lazy-loaded on first request and | ||
| * cached for subsequent calls. Unknown grammars fall back to plaintext. | ||
| */ | ||
| export async function highlightCode( | ||
| code: string, | ||
| lang: string | undefined, | ||
| theme: ShikiTheme = 'light', | ||
| ): Promise<HighlightResult | null> { | ||
| if (code.length > MAX_SHIKI_BYTES) return null; |
Contributor
There was a problem hiding this comment.
MAX_SHIKI_BYTES is compared against code.length (UTF-16 code units), not bytes.
For ASCII the cap is effectively 64 KB, but a CJK-heavy snippet at the same string length is ~2–3× the byte count, so the actual main-thread budget the doc-comment promises is off for non-Latin content. Either rename to MAX_SHIKI_CHARS or compute new TextEncoder().encode(code).byteLength (paying a small one-shot cost) so the threshold lines up with the rationale in the comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/markdown/src/shiki.ts` around lines 117 - 151, The current check in
highlightCode compares code.length (UTF-16 units) to MAX_SHIKI_BYTES, which
misrepresents actual byte size; change the comparison to use the UTF-8 byte
length by computing const byteLen = new TextEncoder().encode(code).length and
compare byteLen > MAX_SHIKI_BYTES, or alternatively rename MAX_SHIKI_BYTES to
MAX_SHIKI_CHARS and keep the existing char-based check; update the
MAX_SHIKI_BYTES constant name/usages and any comments accordingly so the symbol
(MAX_SHIKI_BYTES) and function (highlightCode) reflect the chosen meaning.
Comment on lines
+181
to
+207
| const loaded = highlighter.getLoadedLanguages(); | ||
| if (!loaded.includes(resolvedLang)) { | ||
| try { | ||
| await highlighter.loadLanguage( | ||
| /* @vite-ignore */ import( | ||
| `shiki/langs/${resolvedLang}.mjs` | ||
| ) as Parameters<HighlighterCore['loadLanguage']>[0], | ||
| ); | ||
| } catch (err) { | ||
| console.warn( | ||
| `[shiki] language "${resolvedLang}" not loadable, falling back to plaintext:`, | ||
| err, | ||
| ); | ||
| try { | ||
| return { | ||
| html: highlighter.codeToHtml(code, { | ||
| lang: 'text', | ||
| theme: resolvedTheme, | ||
| }), | ||
| language: 'text', | ||
| }; | ||
| } catch (htmlErr) { | ||
| console.warn('[shiki] plaintext fallback failed:', htmlErr); | ||
| return null; | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Validate resolvedLang before passing it to a dynamic import().
resolveLanguage only lowercases and aliases — it doesn't constrain the character set. With /* @vite-ignore */ the bundler skips its glob-restriction analysis and the specifier is resolved at runtime, so a code-fence info string like ../core would lowercase cleanly and attempt to load shiki/langs/../core.mjs. The exploit surface is small (the wrong module just fails loadLanguage's shape check), but a one-line whitelist eliminates the class of failure entirely.
🛡️ Proposed guard
+const VALID_LANG_RE = /^[a-z0-9+-]+$/;
+
const loaded = highlighter.getLoadedLanguages();
-if (!loaded.includes(resolvedLang)) {
+if (!loaded.includes(resolvedLang)) {
+ if (!VALID_LANG_RE.test(resolvedLang)) {
+ return {
+ html: highlighter.codeToHtml(code, { lang: 'text', theme: resolvedTheme }),
+ language: 'text',
+ };
+ }
try {
await highlighter.loadLanguage(
/* `@vite-ignore` */ import(
`shiki/langs/${resolvedLang}.mjs`
) as Parameters<HighlighterCore['loadLanguage']>[0],
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const loaded = highlighter.getLoadedLanguages(); | |
| if (!loaded.includes(resolvedLang)) { | |
| try { | |
| await highlighter.loadLanguage( | |
| /* @vite-ignore */ import( | |
| `shiki/langs/${resolvedLang}.mjs` | |
| ) as Parameters<HighlighterCore['loadLanguage']>[0], | |
| ); | |
| } catch (err) { | |
| console.warn( | |
| `[shiki] language "${resolvedLang}" not loadable, falling back to plaintext:`, | |
| err, | |
| ); | |
| try { | |
| return { | |
| html: highlighter.codeToHtml(code, { | |
| lang: 'text', | |
| theme: resolvedTheme, | |
| }), | |
| language: 'text', | |
| }; | |
| } catch (htmlErr) { | |
| console.warn('[shiki] plaintext fallback failed:', htmlErr); | |
| return null; | |
| } | |
| } | |
| } | |
| const VALID_LANG_RE = /^[a-z0-9+-]+$/; | |
| const loaded = highlighter.getLoadedLanguages(); | |
| if (!loaded.includes(resolvedLang)) { | |
| if (!VALID_LANG_RE.test(resolvedLang)) { | |
| return { | |
| html: highlighter.codeToHtml(code, { lang: 'text', theme: resolvedTheme }), | |
| language: 'text', | |
| }; | |
| } | |
| try { | |
| await highlighter.loadLanguage( | |
| /* `@vite-ignore` */ import( | |
| `shiki/langs/${resolvedLang}.mjs` | |
| ) as Parameters<HighlighterCore['loadLanguage']>[0], | |
| ); | |
| } catch (err) { | |
| console.warn( | |
| `[shiki] language "${resolvedLang}" not loadable, falling back to plaintext:`, | |
| err, | |
| ); | |
| try { | |
| return { | |
| html: highlighter.codeToHtml(code, { | |
| lang: 'text', | |
| theme: resolvedTheme, | |
| }), | |
| language: 'text', | |
| }; | |
| } catch (htmlErr) { | |
| console.warn('[shiki] plaintext fallback failed:', htmlErr); | |
| return null; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/markdown/src/shiki.ts` around lines 181 - 207, Validate and restrict
resolvedLang before using it in the dynamic import: in the block that checks
loaded.includes(resolvedLang) and calls highlighter.loadLanguage(...
import(`shiki/langs/${resolvedLang}.mjs`) ...), add a whitelist/validation step
(e.g., allow only a safe token regex like /^[a-z0-9_-]+$/ or check membership
against a known list of shiki language ids) and bail out to the plaintext
fallback (same behavior as the existing catch) if validation fails; reference
resolvedLang, resolveLanguage, and highlighter.loadLanguage when locating the
code to change.
| * with that; chat callers stream large code blocks via their own | ||
| * debounced highlighter passed through `components`. | ||
| */ | ||
| const STREAMING_BASE = baseComponents as unknown as MarkdownComponentMap; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect MarkdownComponentMap and MarkdownComponentType to see how they relate to react-markdown's Components.
fd -t f 'types.ts' packages/markdown/src --exec cat {}
echo "---"
rg -nP -C2 '\b(MarkdownComponentMap|MarkdownComponentType)\b' packages/markdown/srcRepository: tale-project/tale
Length of output: 4316
🏁 Script executed:
fd -t f -e ts -e tsx 'markdown.ts' packages/markdown/src --exec cat {}Repository: tale-project/tale
Length of output: 47805
Avoid the as unknown as double cast — align the types instead.
as unknown as MarkdownComponentMap is exactly the escape hatch the repo's TypeScript rules disallow. Since MarkdownComponentMap is intentionally designed to mirror react-markdown's Components type (per the oxlint comment in types.ts), either:
- Type
baseComponentsdirectly asMarkdownComponentMapinstead ofComponentsfrom react-markdown, or - Create a thin typed adapter function that converts
ComponentstoMarkdownComponentMapwithout the escape hatch.
Per coding guidelines: "Never use as, any, or unknown in TypeScript — use type guards, generics, discriminated unions, or never instead".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/markdown/src/streaming/incremental-markdown.tsx` at line 50, The
double-cast on STREAMING_BASE (const STREAMING_BASE = baseComponents as unknown
as MarkdownComponentMap) must be removed; either change the declaration/type of
baseComponents to MarkdownComponentMap where it's created or add a small adapter
function (e.g., convertComponentsToMarkdownComponentMap) that accepts
react-markdown Components and returns a properly typed MarkdownComponentMap,
then assign STREAMING_BASE to that adapter's result; update references to
baseComponents and STREAMING_BASE so the types align without using `as unknown
as`.
Comment on lines
+14
to
+16
| globalThis.ResizeObserver = | ||
| MockResizeObserver as unknown as typeof ResizeObserver; | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "setup.ts" | grep -i markdown | head -20Repository: tale-project/tale
Length of output: 95
🏁 Script executed:
cat -n ./packages/markdown/test/setup.tsRepository: tale-project/tale
Length of output: 961
Use vi.stubGlobal() instead of double type assertions for ResizeObserver.
The double assertion as unknown as typeof ResizeObserver violates the TypeScript guideline against using as casts. Vitest provides the vi.stubGlobal() API specifically for this pattern.
Recommended change
class MockResizeObserver {
observe = vi.fn();
unobserve = vi.fn();
disconnect = vi.fn();
}
-globalThis.ResizeObserver =
- MockResizeObserver as unknown as typeof ResizeObserver;
+vi.stubGlobal('ResizeObserver', MockResizeObserver);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| globalThis.ResizeObserver = | |
| MockResizeObserver as unknown as typeof ResizeObserver; | |
| class MockResizeObserver { | |
| observe = vi.fn(); | |
| unobserve = vi.fn(); | |
| disconnect = vi.fn(); | |
| } | |
| vi.stubGlobal('ResizeObserver', MockResizeObserver); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/markdown/test/setup.ts` around lines 14 - 16, Replace the double
type assertion assignment to globalThis.ResizeObserver with Vitest's
vi.stubGlobal API: locate the usage of MockResizeObserver and the
globalThis.ResizeObserver assignment and change it to call
vi.stubGlobal('ResizeObserver', MockResizeObserver) so you no longer use `as
unknown as typeof ResizeObserver` and the global is properly stubbed for tests.
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
1 participant
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
Carves markdown rendering out of
@tale/webuiinto a new@tale/markdownworkspace so web, docs, and platform share one Shiki singleton, one component set, and one styling pass. Streaming primitives from chat (incremental reveal, mid-stream syntax repair, paginated tables, CJK plugin) move in alongside the static surface.Bug fixes
countLines()and Shiki now tokenise the same normalised string so trailing-newline blocks no longer drop the last line number. Storybook regression:LineNumbersWithTrailingNewline.routed-markdownhref detection —mailto:/tel:/protocol-relative URLs stay plain anchors instead of leaking into the router.remend-markdowntilde fences —<details>auto-close logic now counts both ``` and~~~fences.Visual polish
.line, edge-to-edge.<thead>with row-hover at<tr>level +scope=colheaders.fg-muted/fg-subtleso body prose reads cleanly.CodeGrouppanels share the exact sameHighlightedCodebody asCodeBlock— single border, no double-padding ring.IncrementalMarkdowndefaults to a streaming-safe map derived frombaseComponentsso streaming and static prose look identical (minuspre/code/heading wrappers that fight cursor injection).Storybook
@tale/markdownstorybook on port6008with dedicated stories for every export: Markdown, CodeBlock, CodeGroup, Callout (5 tones), Mermaid (flowchart / sequence / state / pie / parse-error), Tabs, Steps, Cards, Accordion, Frame, AnchoredHeading, plus formatting + heading samples and anIncrementalMarkdownreveal demo.postcss.config.mjsadded topackages/ui,packages/webui,packages/markdown, and thereact-packageplop template.storybooktask added toturbo.json; platform moved off the duplicate:6006port to:6009soui+platformcan run side-by-side.bun run storybookruns all of them, plusstorybook:ui/storybook:webui/storybook:markdown/storybook:platform.Wiring
@tale/markdownadded to root workspaces and todocs/web/platformdeps.@tale/webuidrops markdown subpath exports + matching deps (react-markdown,remark-gfm,rehype-raw,shiki,mermaid).services/docsswaps<Markdown>→<RoutedMarkdown>so SPA navigation survives the migration; bare<Markdown>stays router-agnostic for storybook / SSR / non-routed contexts.services/platform'sshiki.tsandmarkdown-types.tscollapse to thin re-exports of the shared package; chat call sites adapt to theHighlightResultshape.services/platform/.gitignoreno longer needs storybook entries:.gitignoreaddsstorybook-static/repo-wide.Test plan
bun run check— format, lint, typecheck, all 41 turbo tasks across the repo green (3025 platform tests + 231 markdown tests).bun run --filter @tale/markdown storybook— open:6008, verify dark/light toggle re-themes the page, code blocks have line numbers, callouts/tabs/steps/cards render in both modes, mermaid stories render and the parse-error story stops accumulating fragments on revisit.bun run --filter @tale/docs dev— open a doc page with a fenced code block and confirm: line numbers stay 1:1 with rendered lines, internal links navigate via SPA, GFM alerts render as styled callouts.bun run --filter @tale/platform dev— chat conversations still stream prose correctly; canvas / message-bubble code blocks still highlight via the shared Shiki singleton.Deferred follow-ups
services/web/app/pages/legal-page.tsxto use shared<Markdown>(small isolated surface, low duplication risk).accessibilitydescribe blocks to markdown package stories matching the platform pattern.strict: truefor the markdown package (tsconfig sweep, separate PR).Summary by CodeRabbit
New Features
Improvements