Repository navigation
Conversation
…Mermaid, JoyPixels
|
@itznan is attempting to deploy a commit to the Developer Labs Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ThisIs-Developer
added
accessibility
enhancement
ChatGPT Codex Connector
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… Docker and SW (P3)
…ebounce renders (P6, P8)
…yles, theme contrast, and emoji tones (P1, P2, P4, P5, P7, P8)
This comment was marked as outdated.
This comment was marked as outdated.
|
Hi @itznan, First of all, thank you for resolving all the issues. That’s really great, and I appreciate the amount of work you’ve put into this. Coming to the question of whether we should continue with this feature—yes, I’d like to continue with it. The perspective you shared also makes sense to me. This does not seem like a heavy feature that would significantly affect the overall performance of the application, especially since the additional themes and fonts are loaded only when needed. Many other editors and applications also provide this kind of customization, so I think it makes sense for Markdown Viewer to support it as well. From a UX and user perspective, I tested the latest version again and everything looks much better now. I’ve also given the updated code to Codex for another code review, and I’ll review those results and share the findings with you as soon as possible. Thanks again for picking up this issue, continuing with the feature, and actually making it real. I really appreciate the effort you’ve put into it. |
ThisIs-Developer
left a comment
ThisIs-Developer
left a comment
There was a problem hiding this comment.
Codex review — six confirmed issues
Reviewed PR #278 at e5a4a6104022cf0aeafccd52819fa6bf44f6627f against base 9ce2de94c167deb76ccadbb976654791a059d5e9. The PR head was rechecked before posting and is unchanged. The six current findings are attached inline with reproduction details, observed behavior, and suggested fixes.
| # | Priority | Finding | Code reference |
|---|---|---|---|
| 1 | P1 | Keep the desktop MathJax bundle compatible with the new configuration | desktop-app/resources/js/script.js:4144–4145 |
| 2 | P2 | Switch math font resources along with the font class | visual-settings-adapters.js:325–326 |
| 3 | P2 | Resolve automatic code colors against the PDF export theme | visual-settings-adapters.js:190–195 |
| 4 | P2 | Remove the built-in token palette when applying a syntax theme | visual-settings-adapters.js:217–223 |
| 5 | P2 | Canonicalize emoji aliases before adding a skin-tone suffix | visual-settings-adapters.js:444–450 |
| 6 | P2 | Load Modern when resetting after an alternate-font reload | visual-settings-adapters.js:292–296 |
Validation and coverage
npm run build: passed.npm run test:unit: 32/32 passed.- Chromium/Chrome Playwright coverage across
visual-settings.spec.js,export.spec.js,rich-content.spec.js,startup-loading.spec.js, andreview-mode.spec.js: 34/34 passed. - Additional browser checks used the real rendering libraries and inspected network responses, computed token colors, font classes/resources, and rendered output. These checks exposed failures that the existing passing tests do not assert.
| Feature | Additional checks | Result |
|---|---|---|
| Syntax highlighting | All 15 choices in light and dark app modes; identical highlighted JavaScript compared with the selected stylesheet in isolation | All stylesheets loaded; 10 choices had token-color conflicts in both modes (finding 4) |
| MathJax | All 10 font choices; live switching, reload, representative equations, and reset | Live resource switching and reset failures confirmed (findings 2 and 6) |
| Desktop LaTeX compatibility | Base versus PR configuration using the actual bundled MathJax 3.2.2 dependency | Default math startup regression confirmed (finding 1) |
| PDF export | Auto syntax highlighting with light app/dark export and dark app/light export; base comparison | Code-block export theme regression confirmed (finding 3) |
| Mermaid | 6 themes × 2 looks × 2 app modes × 4 diagram types (flowchart, sequence, class, pie): 96 rendered SVGs; representative reload/reset | All tested SVGs rendered with valid dimensions and no console/page errors; no additional Mermaid defect confirmed |
| Emoji | Default plus 5 skin tones in both app modes; canonical names, aliases, explicit tones, unsupported names, native Unicode, and inline code | Canonical examples followed the setting; aliases failed (finding 5); explicit tones and protected/unsupported content were preserved |
Mermaid's HandDrawn appearance varies by supported diagram type; an unchanged sequence/pie appearance alone was not treated as a PR regression.
Assessment: the feature is not ready to be described as working for every option. These are all six confirmed findings from this review, not a guarantee that no other defects exist. Browser verification was performed in Chromium/Chrome with representative fixtures; Firefox, WebKit, every document/option combination, and a packaged native desktop run were outside the verified coverage.
| chtml: { | ||
| font: initialFont |
There was a problem hiding this comment.
[P1] Keep the desktop MathJax bundle compatible with the new configuration
The desktop configuration now passes a font name string to chtml.font, but desktop-app/prepare.js:274–275 still downloads MathJax 3.2.2 and the desktop loader uses that local bundle. MathJax 3 expects a font object here. This breaks ordinary desktop LaTeX rendering even when the user leaves the default font selected.
Reproduction: render $$x^2$$ using the desktop dependency version with this configuration. In an isolated real-library comparison, the base configuration resolves startup and renders one equation; the PR configuration reports MathJax(?): t.font.adaptiveCSS is not a function, leaves startup pending, never exposes typesetPromise, and renders zero equations. The application harness with the same dependency substitution also leaves raw LaTeX visible.
Suggested fix: update the desktop bundle and configuration together, or use version-compatible configuration for the existing desktop dependency. This was reproduced against the actual 3.2.2 library in a browser harness; a packaged native desktop application was not run.
| if (fontClass) { | ||
| window.MathJax.config.chtml.fontData = fontClass; |
There was a problem hiding this comment.
[P2] Switch math font resources along with the font class
Changing chtml.fontData and rebuilding the output jax retains font resource configuration from the previous font. The selected font class changes, but its glyphs are requested from the wrong package, so the live preview does not reliably render the selected mathematical font.
Reproduction: start with Modern and a rendered equation, then select STIX Two without reloading. The renderer becomes MathJaxStix2, but its fontURL remains https://cdn.jsdelivr.net/npm/mathjax-modern-font/chtml/woff; requests such as .../mathjax-modern-font/chtml/woff/mjx-stx-zero.woff return 404. All nine alternate fonts showed failed live font loads in the Modern-boot matrix. A further case—reload with STIX, then switch to Asana in a document containing \mathbb{R}—reduced five rendered equations to zero and rejected startup with Error: dynamic file 'double-struck' failed to load.
Suggested fix: initialize the selected font's complete resource configuration, including its WOFF URL and dynamic font resources, before rebuilding/typesetting. Verify loaded glyph resources and actual rendered equations; checking only the selected setting or font class misses this failure.
| @media print { | ||
| .markdown-body pre:has(> code.hljs), | ||
| .markdown-body pre > code.hljs, | ||
| .markdown-body code.hljs { | ||
| background-color: ${bg} !important; | ||
| color: ${fg} !important; |
There was a problem hiding this comment.
[P2] Resolve automatic code colors against the PDF export theme
These global !important code-block rules use colors resolved from the application theme and also apply to the export document. With the syntax theme set to Auto, they override the selected PDF theme's code-block styling.
Reproduction: use the light application theme and Auto syntax highlighting, then choose a dark PDF export: the page is dark but code blocks retain a white background. Reversing the themes produces dark code blocks on the light export. The same fixture using the base commit's application files produced the expected code-block colors in both directions.
Suggested fix: resolve Auto against the requested export theme for export rendering, or scope preview-only helper rules so they do not override the export styles.
| let link = document.getElementById('mdv-hljs-theme-link'); | ||
| if (!link) { | ||
| link = document.createElement('link'); | ||
| link.id = 'mdv-hljs-theme-link'; | ||
| link.rel = 'stylesheet'; | ||
| link.crossOrigin = 'anonymous'; | ||
| if (document.head) document.head.appendChild(link); |
There was a problem hiding this comment.
[P2] Remove the built-in token palette when applying a syntax theme
Appending the selected theme stylesheet leaves the existing token colors in styles.css:4115–4204 active. More-specific selectors such as .hljs-title.function_ win over the new theme, and tokens without an explicit rule in the selected theme can retain the old palette. The stylesheet loads and the background changes, but the resulting code colors only partially match the selection.
Reproduction: select Monokai and render a JavaScript function such as function hello() { return 42; }. The function name keeps the legacy purple rgb(111, 66, 193) instead of Monokai's green rgb(166, 226, 46). Comparing the same highlighted HTML against an isolated iframe containing only the selected stylesheet confirmed mismatches in 10 of 15 choices, in both light and dark app modes: Monokai, Dracula, Atom One Dark, Atom One Light, Solarized Dark, Solarized Light, VS, VS2015, Nord, and Default. All stylesheet URLs loaded successfully; this is a cascade conflict.
Suggested fix: scope or disable the built-in token palette while an external theme is active, and compare actual token colors in regression coverage rather than asserting only that the link URL changed. The other five choices matched the representative JavaScript fixture; that does not cover every language/token.
| const toneCandidate = `${shortcode}_${_activeEmojiSkinTone}`; | ||
| const joy = typeof joypixels !== 'undefined' ? joypixels : (typeof window !== 'undefined' ? window.joypixels : undefined); | ||
| if (joy && joy.emojiList) { | ||
| if (joy.emojiList[`:${toneCandidate}:`]) { | ||
| return toneCandidate; | ||
| } | ||
| return shortcode; |
There was a problem hiding this comment.
[P2] Canonicalize emoji aliases before adding a skin-tone suffix
This lookup adds _toneN to the original shortcode without first resolving JoyPixels aliases. A supported emoji can therefore remain at its default tone when its alias is used, even though the equivalent canonical shortcode follows the selected setting.
Reproduction: render :thumbsup: :+1: :runner: and select any of tone1 through tone5. :thumbsup: follows the selected tone, while :+1: stays yellow; :runner: also remains at its default tone. This reproduced for all five non-default tone choices in both light and dark mode. Canonical shortcodes such as :wave: and :thumbsup: worked, and explicitly toned shortcodes were preserved.
Suggested fix: resolve the input to JoyPixels' canonical shortcode before checking for a toned variant, while preserving explicit tones and leaving genuinely unsupported emojis unchanged.
| // Load external font script if needed (Modern is bundled by default) | ||
| if (_activeMathFont !== 'mathjax-modern') { | ||
| const fontUrl = `${MATHJAX_FONT_CDN_BASE}${_activeMathFont}-font/chtml.js`; | ||
| try { | ||
| await loadScriptOnce(fontUrl); |
There was a problem hiding this comment.
[P2] Load Modern when resetting after an alternate-font reload
The assumption that Modern is always bundled is false after startup with a persisted alternate font. In that case only the alternate font package is registered. Skipping Modern's loader means there is no Modern class to install, and the later fontData assignment leaves the previous renderer active.
Reproduction: select STIX Two, reload, then click Reset to defaults. The store, selector, and adapter report mathjax-modern, but the output remains MathJaxStix2 with prefix STX and fontData: MathJaxStix2; the registered font packages still contain only mathjax-stix2. A minimal $$x^2$$ document reproduces this, independently of the missing-glyph-resource problem in finding 2. Reset after a Termes reload likewise retained Termes.
Suggested fix: load the Modern package whenever it is absent, including reset/fallback paths, and verify that the active renderer class and font resources match the restored setting.
|
Hi @itznan, I know I’ve shared multiple rounds of feedback. I’m only doing this because I don’t want the feature to become slow or unstable — I want it to be as good as possible before merging. Thank you again for all the work you’re putting into it. Really appreciate it. |
|
Thanks @ThisIs-Developer, no problem at all. These were real issues and I’m glad they were caught before merging. I’ll fix all six (P1 desktop MathJax first) and add tests that verify rendered output. Will update the PR soon. |
Summary
Implements a configurable visual library settings panel allowing users to customize rendering appearances for highlight.js, MathJax, Mermaid, and JoyPixels emojis. Settings update live without full page reload, persist across reloads via
localStorage(mdv.visualSettings.v1), and support light/dark modes and mobile viewports.Closes #129
Configurable Settings & Options
hljsTheme):auto(follows app light/dark mode) + 14 themes (github,github-dark,monokai,dracula,atom-one-dark,atom-one-light,solarized-dark,solarized-light,vs,vs2015,nord,tokyo-night-dark,tokyo-night-light,default).mathFont): 10 mathematical fonts (mathjax-modern,mathjax-tex,mathjax-stix2,mathjax-asana,mathjax-bonum,mathjax-dejavu,mathjax-fira,mathjax-pagella,mathjax-schola,mathjax-termes).mermaidTheme):auto,default,neutral,dark,forest,base.mermaidLook):classic,handDrawn.emojiSkinTone):default(yellow),tone1-tone5(Fitzpatrick modifiers). Preserves explicit tones and leaves unsupported emojis untouched.Library & Architecture Updates
4.0.0-beta.7) with SRI verification (sha384-2f5bAKjuFIbbQ+0O9aSu5W1OyLA4q80QrDEvjXDwJexjU9VqoJhMgw19pYVpSJab) to enable dynamic on-demand mathematical font switching.visual-settings-store.js) from library adapters (visual-settings-adapters.js), with schema validation and subscriber events.Local Testing & Validation
Run locally:
npm install && npm startValidation results:
npm run build: Static build smoke check and SEO sitemap check passed.npm run test:unit: 32/32 unit tests passed (including store and adapter tests).PLAYWRIGHT_CHANNEL=chrome npx playwright test tests/e2e/visual-settings.spec.js: 3/3 passed (dialog lifecycle, live preview updates/persistence/reset, dark mode/mobile).Known Limitations
4.0.0-beta.7.